Horton

Thread

Commits

  1. A minor simplification for List manipulation

  1. Improve list manipulation in several places

    Richard Guo <guofenglinux@gmail.com> — 2023-04-21T07:34:42Z

    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

  2. Re: Improve list manipulation in several places

    Ranier Vilela <ranier.vf@gmail.com> — 2023-04-21T11:16:01Z

    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

  3. Re: Improve list manipulation in several places

    David Rowley <dgrowleyml@gmail.com> — 2023-04-21T12:09:54Z

    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

  4. Re: Improve list manipulation in several places

    Ranier Vilela <ranier.vf@gmail.com> — 2023-04-21T16:11:56Z

    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

  5. Re: Improve list manipulation in several places

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-04-21T16:55:14Z

    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.

  6. Re: Improve list manipulation in several places

    Richard Guo <guofenglinux@gmail.com> — 2023-04-23T06:42:53Z

    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

  7. Re: Improve list manipulation in several places

    Richard Guo <guofenglinux@gmail.com> — 2023-04-23T06:57:35Z

    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

  8. Re: Improve list manipulation in several places

    Tender Wang <tndrwang@gmail.com> — 2023-04-23T10:13:41Z

    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).



>
> (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
>

  9. Re: Improve list manipulation in several places

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-05-08T15:22:28Z

    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.

  10. Re: Improve list manipulation in several places

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-08T17:25:58Z

    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/

  11. Re: Improve list manipulation in several places

    Ranier Vilela <ranier.vf@gmail.com> — 2023-05-08T17:48:28Z

    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

  12. Re: Improve list manipulation in several places

    Richard Guo <guofenglinux@gmail.com> — 2023-05-09T03:07:47Z

    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

  13. Re: Improve list manipulation in several places

    Richard Guo <guofenglinux@gmail.com> — 2023-05-09T03:13:44Z

    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

  14. Re: Improve list manipulation in several places

    Richard Guo <guofenglinux@gmail.com> — 2023-05-09T03:15:42Z

    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

  15. Re: Improve list manipulation in several places

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-09T08:01:38Z

    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/

  16. Re: Improve list manipulation in several places

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-07-03T09:41:07Z

    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.

  17. Re: Improve list manipulation in several places

    Richard Guo <guofenglinux@gmail.com> — 2023-07-04T00:48:00Z

    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