Thread: Improve list manipulation in several places

There was discussion in [1] about improvements to list manipulation in
several places.  But since the discussion is not related to the topic in
that thread, fork a new thread here and attach a patch to show my
thoughts.

Some are just cosmetic changes by using macros.  The others should have
performance gain from the avoidance of moving list entries.  But I doubt
the performance gain can be noticed or measured, as currently there are
only a few places affected by the change.  I still think the changes are
worthwhile though, because it is very likely that future usage of the
same scenario can benefit from these changes.

On 21.04.23 09:34, Richard Guo wrote:
> There was discussion in [1] about improvements to list manipulation in
> several places.  But since the discussion is not related to the topic in
> that thread, fork a new thread here and attach a patch to show my
> thoughts.
>
> Some are just cosmetic changes by using macros.  The others should have
> performance gain from the avoidance of moving list entries.  But I doubt
> the performance gain can be noticed or measured, as currently there are
> only a few places affected by the change.  I still think the changes are
> worthwhile though, because it is very likely that future usage of the
> same scenario can benefit from these changes.

Can you explain the changes?

Maybe this patch should be split up.  It seems some of the changes are
trivial simplifications using existing APIs, while others introduce new
functions.

+lcons_copy(void *datum, const List *list)

+lappend_copy(const List *list, void *datum)

list param pointer can be const here not?

There was discussion in [1] about improvements to list manipulation in
several places.  But since the discussion is not related to the topic in
that thread, fork a new thread here and attach a patch to show my
thoughts.

Some are just cosmetic changes by using macros.  The others should have
performance gain from the avoidance of moving list entries.  But I doubt
the performance gain can be noticed or measured, as currently there are
only a few places affected by the change.  I still think the changes are
worthwhile though, because it is very likely that future usage of the
same scenario can benefit from these changes.

(Copying in David and Ranier.  Ranier provided a patch about the changes
in list.c, but I'm not using that one.)

[1] https://www.postgresql.org/message-id/CAMbWs49aakL%3DPP7NcTajCtDyaVUE-NMVMGpaLEKreYbQknkQWA%40mail.gmail.com

Thanks
Richard

On 2023-May-08, Peter Eisentraut wrote:

> On 23.04.23 08:42, Richard Guo wrote:
> > Thanks for the suggestion.  I've split the patch into two as attached.
> > 0001 is just a minor simplification by replacing lfirst(list_head(list))
> > with linitial(list).  0002 introduces new functions to reduce the
> > movement of list elements in several places so as to gain performance
> > improvement and benefit future callers.
>
> These look sensible to me.  If you could show some numbers that support the
> claim that there is a performance advantage, it would be even more
> convincing.

0001 looks fine.

The problem I see is that each of these new functions has a single
caller, and the only one that looks like it could have a performance
advantage is list_copy_move_nth_to_head() (which is the weirdest of the
lot).  I'm inclined not to have any of these single-use functions unless
a performance case can be made for them.

On 23.04.23 08:42, Richard Guo wrote:
> Thanks for the suggestion.  I've split the patch into two as attached.
> 0001 is just a minor simplification by replacing lfirst(list_head(list))
> with linitial(list).  0002 introduces new functions to reduce the
> movement of list elements in several places so as to gain performance
> improvement and benefit future callers.

These look sensible to me.  If you could show some numbers that support
the claim that there is a performance advantage, it would be even more
convincing.

The problem I see is that each of these new functions has a single
caller, and the only one that looks like it could have a performance
advantage is list_copy_move_nth_to_head() (which is the weirdest of the
lot).  I'm inclined not to have any of these single-use functions unless
a performance case can be made for them.

I think you missed list_nth_xid, It makes perfect sense to exist.

On 09.05.23 05:13, Richard Guo wrote:
> Yeah, maybe this is the reason I failed to devise a query that shows any
> performance gain.  I tried with a query which makes the 'all_pathkeys'
> in sort_inner_and_outer being length of 500 and still cannot see any
> notable performance improvements gained by list_copy_move_nth_to_head.
> Maybe the cost of other parts of planning swamps the performance gain
> here?  Now I agree that maybe 0002 is not worthwhile to do.

I have committed patch 0001.  Since you have withdrawn 0002, this closes
the commit fest item.