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

On Wed, Jun 20, 2012 at 6:59 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> My guess is that it wouldn't be too hard to remove some of the extra
>> pointers.  Anyone who is using Dllist as a non-inline list could be
>> converted to List * instead.
> There are only three users of the whole dllist.h. Catcache, autovacuum and
> postmaster. The latter two just keep a list of databases around. So any change
> will only be moderatively intrusive.

Yeah.

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

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?

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

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

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company