Hi Robert, > 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.
OK, thanks for the heads-up. > 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? No, this patch will benefit any architecture that has a gcc implementation of __builtin_clz. I know that both x86 and powerpc gcc support this. > 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? Yes, Atsushi Ogawa did some benchmarking with this patch on x86, his numbers are here: http://archives.postgresql.org/message-id/4a2895c4.9050...@hi-ho.ne.jp In fact, Atushi originally submitted a patch using inline asm (using "bsr") to do this on x86. Coincidentally, I was working on a powerpc implementation (using "cntlz") at the same time, so submitted a patch using the gcc builtin that would work on both (and possibly other) architectures. > 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. The 2% improvement I saw is for a sysbench OLTP run. I'm happy to re-do the run and report specific numbers if you need them. > 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) OK, happy to make these changes. What's the commitfest process here, should I redo the patch and re-send to -hackers? (inline again: should I just make this a static, the compiler can inline where possible? or do you want a macro?) > > And general comment: > > > > Do we want to have this kind of code which is optimized for one > > compiler and platform. One compiler, multiple platforms. > > 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 don't think we're going to see a lot of variation here (besides the difference where __builtin_clz isn't available). Given that this is implemented with a couple of instructions, I'm confident that we won't see any degradation for platforms that support __builtin_clz. For the other cases, we just use the exiting while-loop algorithm so there should be no change (unless we mess up the inlining...). > > 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. Unless I'm missing something, spinlocks are done the same way - we have one file, src/include/storage/s_lock.h, which is mostly different implementations of the lock primitives for different platforms, separated by preprocessor tests. Essentially, this is just one line of code, protected by HAVE_BUILTIN_CLZ (which is a feature-test, rather than a specific platform or compiler test), and is only used in one place. I don't think that warrants a separate file. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
