Hi Jakub, Thank you for testing and for your feedback. I've tried to address some of the questions you raised, as follows:
a. SPINS_PER_LOCK_READ_THRESHOLD is a threshold reflecting how severe the lock contention is. It is set to 5 here, meaning the spin loop read lock->state 5 times before observing the free state; more than 5 reads is considered heavy contention, so we use spins_per_lock_read + 5 to ease the contention. (Although both values are 5, they mean different things.) Setting SPINS_PER_LOCK_READ_THRESHOLD to 3, 5, or 6 can all improve performance-there is no strict requirement. The optimal value may differ across CPU core counts or models, but 5 is a value that shows improvement on all the platforms tested so far. a2. 256 is an empirically chosen upper bound that proved large enough across different platforms and CPU core counts; it isn't a precise limit-values like 300 or 500 would work just as well, as long as it isn't too small. a3. Yes, your approach probably gives finer-grained control. b. When spins_per_lock_read is shared across all LWLocks, its value is effectively determined by the most heavily contended lock - which is also where the biggest performance hotspot is. The current spins_per_lock_read may be too large for another LWLock and add some latency, but the gain from relieving the most contended lock should outweigh that. Making it per-tranche would give finer control, at the cost of more complex code changes and an uncertain overall performance benefit. c. Yes, your description is more accurate. d. Thanks, that's a good suggestion. For now, though, let's first see whether our current approach gets accepted. Best Regards, -Baohong -----Original Message----- From: Jakub Wartak <[email protected]> Sent: Thursday, October 1, 2026 5:23 PM To: Min, Baohong <[email protected]> Cc: [email protected]; Okanovic, Haris <[email protected]>; PostgreSQL Hackers <[email protected]> Subject: 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(r) 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
