Re: [PATCH] Reduce LWLockWaitListLock() cache-line contention with adaptive spin reads - Mailing list pgsql-hackers

Hi Baohong,

thanks for touching such advanced

> Thanks to Haris for adding the AWS Graviton performance tests - we now have performance data on both Intel (x86) and
Arm.
> Wenhui, could you please review whether this patch is ready to commit?

I've tried my best here. This patched looked interesting and WOW! I've got
extreme +23% with this patch in artificial highly contended LWLock scenarios
(Wow-because judging from the lwlock.c and s_lock.h code and reading some
previous discussion about LWLocks/ spinlocks this stuff looks like already
super-optimized).

In normal pgbench (-S), I've got +12%. Of course the effects seem to mostly
present on >=2 socket systems mainly.

I LWLock microbenchmark
=======================
I've tried this on couple systems, on laptop or single socket systems (single
CCD) it doesn't move a needle (good!), but then I've primarly tested this on
i4i.metal EC2 VM (Ice Lake, 2s64c128t, 2 NUMA nodes) with LWLock/Xidgen
caused by wal_insert.sql and "pgbench -n -c 500 -j 128 -T 20 -P 1 -f
wal_insert.sql" along with:
- wal_buffers=256MB
- and fsync=off (to avoid LWLock/WALWrite)

Where wal_insert.sql was (and t had no pk index):
  \set id random(1, 1000000000)
  INSERT INTO t VALUES (:id, repeat('x', 100));

With perf with the patch one can see LWLockWaitListLock() disapearing as
top symbol. So in that stress-test this yields mentioned +21% in high
contention scenarios (here anything >= 128 VCPUs), it is avg of 3 runs:

clients master patch delta
32      153k   154k  +1%
128     180k   201k  +12%
256     172k   212k  +23%
500     165k   202k  +21%
1024    160k   164k  +2%

500 not 51,* because it was just first try at this
at artificial > 500 (so the -c 1024 runs) there's way more
IPC/ProcarrayGroupUpdate, but that's another unrelated (futex scalability?)
problem, but it explains why benefit is much smaller there.

The patch comes with two changes and when trying out those with -c 500
to quantify their benefit:
a) s_lock.c (MIN_DELAY_USEC) change alone gives + ~6%
b) lwlock.c (adaptive re-read), alone gives + ~17%
c) both, as mentioned above +21%, btw this also allows WAL generation rate
   to also  increase from like 660MB to 790MB/s :o (but that's fsync=off)

So to my understanding this patch is major reduction the of cache-line
ping-pongs that occurs between sockets(L3s/CCDs) under wait list
is being altered (contended). When researching this further, it looked like
this patch brings us closer to what is described in  [1] "Intel® 64 and
IA-32 Architectures Optimization Reference Manual, v50, Volume 1" on page
81 "2.7.4 PAUSE LATENCY IN SKYLAKE CLIENT MICROARCHITECTURE" /
"Example 2-10. Contended Locks with Increasing Back-off" (re-reading lock
with atomics cmpxchng sometimes, and not always?).

Because the above goes into details about CPU microarchitectures, I've gave
a shot into spotting regressions on on my legacy 4s32c64t NUMA Xeon Sandy
Bridge EP and with quick shot test with that wal_insert.sql it gave me much
smaller benefits, but still observable:

clients master patch delta
32      151k   152k  0.5%
64      161k   166k  3%
128     129k   140k  ~9%
500     95k    99k   3%

so there is no regression even on very old hardware. I don't have access to
multi-sockets ARM/PowerPC to quanitfy the benefits there.

II normal pgbench
=================
So with promising results above, I've gave a short to classic pgbench (-S)
too. Back to that modern box i4i.metal EC2 VM (1TB RAM) with s_b=16GB and just
~100GB (-s 7000) in VFS cache that to causing stress of LWLock/BuffeMappings:
multiple runs of "pgbench -n -S -c 500 -j 128 -T 20"" report +12% (!) on fully
isolated/stabilized hw. The perf reports drop of LWLockWaitListLock() from
~4% to non-existent levels (we are talking boost from 1136k to 1273k TPS, I'm
quite impressed as I haven't such big performance jump with such small patch
for quite a while!)

III patch itself
================
I've this to commitfest as apparently it was missing from there, so people
could miss this. It's tracked as https://commitfest.postgresql.org/patch/7376/
(but just 2 with authors, I couldn't find )

Some review findings (and questions!):

a. This: +#define SPINS_PER_LOCK_READ_THRESHOLD  5 makes some sense to me
   (number 3 would also make sense :D), but anyway, then later in
   LWLockWaitListLock() we have:
   +  /* Adaptively adjust read interval based on wait duration */
   +  if (lock_read_count > SPINS_PER_LOCK_READ_THRESHOLD)
   +    spins_per_lock_read = Min(spins_per_lock_read + 5, 256);

   shouldn't it be read:
   spins_per_lock_read = Min(spins_per_lock_read+SPINS_PER_LOCK_READ_THRESHOLD
   instead ?

a2.and also how the "256" was derrived there? (that seems to 256x PAUSE
   instructions? which is somehow is bound to the Intel ?? and that earlier
   mentioned Intel doc is explict about different PAUSE instruction even on
   various Intel's microarchitectures ??? // pre vs after Skylake), If that's
   true perhaps we should somehow also cover other architectures and perhaps
   somehow use s_lock.h / pg_cpu*[.ch] to track all of that? Dunno, just
   asking? This would be also solution/in line with what Haris observed in [2]
   where he writes that his ARM LWLockWaitListLock() change causes regression
   on Intel/AMD "Intel Granite Rapids and AMD Turin (x86_64) both show minor
   degradation with the change, which is the reason the patch is currently
   limited to arch64 only"

a3.This stuff is pretty hard to reason about and test, but hypothethically if
   we perform_spin_delay() and the pg_usleeps() in the edge case gets nearly
   maximums (MAX_DELAY_USLEEP = 1s), and we get spins_per_lock_read nearby to
   256 (in increments of 5), wouldn't that mean we are spinning with stale
   cached reads potentially without a valid reason? Shouldn't we force atomic
   re-read immediatlely once after we have really slept (context-switch) with
   some long pg_usleep()? (it could have change by then, true/false?) It's
   hard to reason because now there are two independent semi-time-tracking
   things at once: spins_per_lock_read + delayStatus.cur_delay. This is more
   of question.

b. isn't the common one global static variable "spins_per_lock_read" combined
   for all lwlocks good? (I'm afraid of situation, where single condented
   LWLock type cascades value to the other ones, shouldn't this be at least
   per tranche? in the artificial scenarios we simply simulate one giant
   contention, but, I'm afraid if some production systems are having multiple
   contentions all at the same time). I'm talking about this line:
      +static int spins_per_lock_read = 1;

c. nitpicking :) -- commitmsg says "re-read lock->state on every iteration,
   causing heavy cache-line bouncing that limits throughput", perhaps it
   should say "atomic re-reads of ..." because pure reading doesn't seem to
   cause cache-ling ping pongs.

d. There seem to be other places than just LWLockWaitListLock() that also
   seem to have the same pattern of using perform_spin_delay() in this
   pattern:
      while(somestate & SOME_FLAG) {
         perform_spin_delay()
         somestate = pg_atomic_read_u64(...)
      }
   dunno, but if we are fixing LWLockWaitListLock(), wouldn't it make some
   sense to improve also bufmgr.c's ones: LockBufHdr(), WaitBufHdrUnLock() ?
   It's an open question / idea and I haven't tried, maybe someone know if
   fixing that along they one too wouldn't boost the rightmost monotonically
   increasing inserts scenarios? (we would need to be hitting single block
   often in NUMA systems to observe this?? or maybe some other scenario?)

-J.

[1] -
https://www.intel.com/content/www/us/en/developer/articles/technical/intel64-and-ia32-architectures-optimization.html
[2] -
https://www.postgresql.org/message-id/DM6PR18MB29081469262A7BBCE85220B3A8112%40DM6PR18MB2908.namprd18.prod.outlook.com