Re: [PATCH v3] Avoid manual shift-and-test logic in AllocSetFreeIndex

Robert Haas wrote:
> On Tue, Jun 30, 2009 at 3:08 AM, Jeremy Kerr<jk@ozlabs.org> wrote:
>> Move the shift-and-test login into a separate fls() function, which
>> can use __builtin_clz() if it's available.
>>
>> This requires a new check for __builtin_clz in the configure script.
>>
>> Results in a ~2% performance increase on PowerPC.
> 
> There are some comments on this patch that have been posted to
> commitfest.postgresql.org rather than emailed to -hackers.  Note to
> all: please don't do this.  The purpose of commitfest.postgresql.org
> is to summarize the mailing list discussion for easy reference, not to
> replace the mailing lists.
> 
> That having been said, Jeremy, you probably want to take a look at
> those comments and I have a few responses to them as well.
> 
> Dan Colish, the round-robin reviewer assigned to this patch, added the
> following comment:
>> Applied and built cleanly. Regress passes. Trying to hunt down ppc box to see if performance enhancement
>> can be seen.
> 
> Question: are we only doing this because of PowerPC?  What is going to
> happen on x86 and other architectures?  x86 is not the center of the
> universe, but at a very minimum we need to make sure that things don't
> go backwards on what is a very common platform.  Has anyone done any
> benchmarking of this?
> 
> A related question: the original email for this patch says that it
> results in a performance increase of about 2% on PPC.  But since it
> gives no details on exactly what the test was that improved by 2%,
> it's hard to judge the impact of this.  If this means that
> AllocSetFreeIndex() is 2% faster, I think we should reject this patch
> and move on.  It's not worth introducing architecture-specific code to
> get a performance improvement that probably won't even move the needle
> on a macro-benchmark.  On the other hand, if the test was something
> like a pgbench run, then this is really very impressive.  But we don't
> know which it is.

There are numbers in the original thread this patch 
http://archives.postgresql.org/pgsql-hackers/2009-06/msg00159.php 
resulted in.

Stefan