Re: [PATCH 04/16] Add embedded list interface (header only)

On Wednesday, June 20, 2012 02:51:30 PM Robert Haas wrote:
> On Wed, Jun 20, 2012 at 6:59 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> >> Also, the performance-critical things
> >> could be reimplemented as macros.
> >>
> >> I question, though, whether we really need both single and doubly linked
> >> lists. That seems like it's almost certainly micro-optimization that we
> >> are better off not doing.
> >
> > It was certainly worthwile for the memory manager (lower per allocation
> > overhead). You might be right that its not worth it for many other
> > possible usecases in pg. Its not much code though.
> >
> > *looks around*
> >
> > A quick grep found:
> >
> > single linked list like code: guc_private.h, aset.c, elog.h, regguts.h
> > (ok, irrelevant), dynahash.c, resowner.c, extension.c, pgstat.c, xlog.c
> > Double linked like code: shmqueue.c, lwlock.c, dynahash.c, xact.c
> >
> > I stopped at that point because while surely not of all of the above
> > usecases could be replaced by a common implementation several could from
> > a quick look. Also, several pg_list.h users could benefit from a
> > conversion. So I think adding a single linked list implementation is
> > worthwile.
>
> I can believe that, although I fear it may be a distraction in the
> grand scheme of getting logical replication implemented. There should
> be very few places where this is actually performance-critical, and
> Tom will complain about large amounts of code churn that don't improve
> performance.
Uh. I don't want to just go around and replace anything randomly. Actually I
don't want to change anything for now except whats necessary to get the patch
in. The point I tried to make was just that the relatively widespread usage of
similar structure make it likely that it can be used in more places in future.

> If we're going to do that, how about transforming dllist.h into the
> doubly-linked list and adding sllist.h for the singly-linked list?
I would be fine with that.

I will go and try to cook up a patch, assuming for now that we rely on inline,
the ugliness can be added back afterwards.

> >> > The most contentious point is probably relying on USE_INLINE being
> >> > available anywhere. Which I believe to be the point now that we have
> >> > gotten rid of some platforms.

> >> I would be hesitant to chuck that even though I realize it's unlikely
> >> that we really need !USE_INLINE. But see sortsupport for an example
> >> of how we've handled this in the recent past.
> >
> > I agree its possible to resolve this. But hell, do we really need to add
> > all that ugliness in 2012? I don't think its worth the effort of support
> > ancient compilers that don't support inline anymore. If we could stop
> > trying to support catering for probably non-existing compilers we could
> > remove some *very* ugly long macros for example (e.g. in htup.h).
>
> I don't feel qualified to make a decision on this one, so will defer
> to the opinions of others.
Ok.

> > If support for !USE_INLINE is required I would prefer to have an header
> > define the functions like
> >
> > #ifdef USE_INLINE
> > #define OPTIONALLY_INLINE static inline
> > #define USE_LINKED_LIST_IMPL
> > #endif
> >
> > #ifdef USE_LINKED_LIST_IMPL
> >
> > OPTIONALLY_INLINE void myFuncCall(){
> > ...
> > }
> > #endif
> >
> > which then gets included with #define USE_LINKED_LIST_IMPL by some c file
> > defining OPTIONALLY_INLINE to something empty if !USE_INLINE.
> > Its too much code to duplicate imo.
>
> Neat trick. Maybe we should revise the sortsupport stuff to do it that
> way.
Either that or at least add a comment to both that its duplicated...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services