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(at)ozlabs(dot)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