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

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