Re: dynamic shared memory and locks

On Tue, Jan 7, 2014 at 6:54 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Maybe it makes sense to have such a check #ifdef'ed out on most builds
>> to avoid extra overhead, but not having any check at all just because we
>> trust the review process too much doesn't strike me as the best of
>> ideas.
>
> I don't think that check would have relevantly high performance impact
> in comparison to the rest of --enable-cassert - it's a single process
> local variable which is regularly accessed. It will just stay in
> L1 or even registers.

I tried it out on a 16-core, 64-hwthread community box, PPC. pgbench
-S, 5-minute rules at scale factor 300 with shared_buffers=8GB:

resultsr.cassert.32.300.300:tps = 11341.627815 (including connections
establishing)
resultsr.cassert.32.300.300:tps = 11339.407976 (including connections
establishing)
resultsr.cassert.32.300.300:tps = 11321.339971 (including connections
establishing)
resultsr.cassert-spin-multiplex.32.300.300:tps = 11492.927372
(including connections establishing)
resultsr.cassert-spin-multiplex.32.300.300:tps = 11471.810937
(including connections establishing)
resultsr.cassert-spin-multiplex.32.300.300:tps = 11516.471053
(including connections establishing)

By comparison:

resultsr.master.32.300.300:tps = 197939.111998 (including connections
establishing)
resultsr.master.32.300.300:tps = 198641.337720 (including connections
establishing)
resultsr.master.32.300.300:tps = 198675.404349 (including connections
establishing)

So yeah, the overhead is negligible, if not negative. None of the
other changes in that patch were even compiled in this case, since all
that code is protected by --disable-spinlocks. Maybe somebody can
find another workload where the overhead is visible, but here it's
not. On the other hand, I did discover another bit of ugliness - the
macros actually have to be defined this way:

+#define SpinLockAcquire(lock) \
+ (AssertMacro(!any_spinlock_held), any_spinlock_held = true, \
+ S_LOCK(lock))
+#define SpinLockRelease(lock) \
+ do { Assert(any_spinlock_held); any_spinlock_held = false; \
+ S_UNLOCK(lock); } while (0)

SpinLockAcquire has to be a comma-separated expression rather than a
do {} while (0) block because S_LOCK returns a value (which is used
when compiling with LWLOCK_STATS); SpinLockRelease, however, has to be
done the other way because S_UNLOCK() is defined as a do {} while (0)
block already on PPC among other architectures.

Overall I don't really care enough about this to argue strenuously for
it. I'm not nearly so confident as Tom that no errors of this type
will creep into future code, but on the other hand, keeping
--disable-spinlocks working reliably isn't significant enough for me
to want to spend any political capital on it. It's probably got a dim
future regardless of what we do here.

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