On Tue, Jun 30, 2009 at 3:08 AM, Jeremy Kerr<j...@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. Zdenek Kotala added this comment: > I have several objections: > > - inline is forbidden to use in PostgreSQL - you need exception or do it > differently > > - type mismatch - Size vs unsigned int vs 32. you should use only Size and > sizeof(Size) > > And general comment: > > Do we want to have this kind of code which is optimized for one compiler and > platform. See openSSL or > MySQL, they have many optimizations which work fine on one platform with > specific version of compiler and > specific version of OS. But if you select different version of compiler or > different compiler you can get very > different performance result (e.g. MySQL 30% degradation, OpenSSL RSA 3x > faster and so on). > > I think in this case, it makes sense to have optimization here, but we should > do it like spinlocks are > implemented and put this code into /port. It should help to implemented > special code for SPARC and SUN > Studio for example. I don't have any thoughts on this part beyond what I stated above, but hopefully Jeremy does. I am going to mark this "Waiting on author" pending a response to all of the above, though hopefully Dan Colish will continue with his reviewing efforts in the meantime. Thanks, ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
