Improve list manipulation in several places

Em sex., 21 de abr. de 2023 às 04:34, Richard Guo <guofenglinux@gmail.com>
escreveu:

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.

+1

Perhaps list_delete_nth_cell needs to check NIL too?
+ if (list == NIL)
+ return NIL;

+lcons_copy(void *datum, const List *list)
+lappend_copy(const List *list, void *datum)
list param pointer can be const here not?

regards,
Ranier Vilela

On Fri, 21 Apr 2023 at 23:16, Ranier Vilela <ranier.vf@gmail.com> wrote:

Perhaps list_delete_nth_cell needs to check NIL too?
+ if (list == NIL)
+ return NIL;

Which cell would you be deleting from an empty list?

David

Em sex, 21 de abr de 2023 9:10 AM, David Rowley <dgrowleyml@gmail.com>
escreveu:

On Fri, 21 Apr 2023 at 23:16, Ranier Vilela <ranier.vf@gmail.com> wrote:

Perhaps list_delete_nth_cell needs to check NIL too?
+ if (list == NIL)
+ return NIL;

Which cell would you be deleting from an empty list?

None.
But list_delete_nth_cel can checks a length of NIL list.

Perhaps a assert?

regards,
Ranier Vilela

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.

On Sat, Apr 22, 2023 at 12:55 AM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

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.

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.

Thanks
Richard

On Fri, Apr 21, 2023 at 7:16 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

+lcons_copy(void *datum, const List *list)
+lappend_copy(const List *list, void *datum)
list param pointer can be const here not?

Correct. Good point. V2 patch does that.

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> 于2023年4月21日周五 15:35写道：

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.

I doubt the performance gain from lappend_copy func. new_tail_cell in
lappend may not enter enlarge_list in most cases, because we
may allocate extra cells in new_list(see the comment in new_list).

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.

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.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Em seg., 8 de mai. de 2023 às 14:26, Alvaro Herrera <alvherre@alvh.no-ip.org>
escreveu:

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.

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

regards,
Ranier Vilela

On Mon, May 8, 2023 at 11:22 PM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> 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.

Thanks Peter for looking at those patches. I tried to devise a query to
show performance gain but did not succeed :-(. So I begin to wonder if
0002 is worthwhile to do, as it seems that it does not solve any real
problem.

Thanks
Richard

On Tue, May 9, 2023 at 1:26 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

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.

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.

Thanks
Richard

On Tue, May 9, 2023 at 1:48 AM Ranier Vilela <ranier.vf@gmail.com> wrote:

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

It seems that list_nth_xid is more about simplification. So maybe we
should put it in 0001?

Thanks
Richard

On 2023-May-08, Ranier Vilela wrote:

Em seg., 8 de mai. de 2023 às 14:26, Alvaro Herrera <alvherre@alvh.no-ip.org>
escreveu:

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.

I saw that one; it's just syntactic sugar, just like list_nth_int and
list_nth_oid, except it has only one possible callsite instead of a
dozen like those others. I see no harm in that function, but no
practical advantage to it either. Xid lists are a very fringe feature,
there being exactly one place in the whole server that uses them.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On 09.05.23 05:13, Richard Guo wrote:

On Tue, May 9, 2023 at 1:26 AM Alvaro Herrera <alvherre@alvh.no-ip.org
<mailto:alvherre@alvh.no-ip.org>> wrote:

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.

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.

On Mon, Jul 3, 2023 at 5:41 PM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

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.

Thanks for pushing it and closing the item!

Thanks
Richard