Hi, On 2025-01-08 12:12:19 -0600, Nathan Bossart wrote: > On Tue, Oct 22, 2024 at 02:54:57PM -0500, Nathan Bossart wrote: > > My colleague Salvatore Dipietro (CC'd) sent me a couple of profiles that > > showed an enormous amount of s_lock() time going to the > > __sync_lock_test_and_set() call in the AArch64 implementation of tas(). > > Upon closer inspection, I noticed that we don't implement a custom > > TAS_SPIN() for this architecture, so I quickly hacked together the attached > > patch and ran a couple of benchmarks that stressed the spinlock code. I > > found no discussion about TAS_SPIN() on ARM in the archives, but I did > > notice that the initial AArch64 support was added [0] before x86_64 started > > using a non-locking test [1]. > > > > These benchmarks are for a c8g.24xlarge running a select-only pgbench with > > 256 clients and pg_stat_statements.track_planning enabled. > > > > without the patch: > > > > [...] > > > > tps = 74135.100891 (without initial connection time) > > > > with the patch: > > > > [...] > > > > tps = 549462.785554 (without initial connection time) > > Are there any objections to proceeding with this change? So far, it's been > tested on a c8g.24xlarge and an Apple M3 (which seems to be too small to > show any effect). If anyone has access to a larger ARM machine, additional > testing would be greatly appreciated. I think it would be unfortunate if > this slipped to v19.
Seems reasonable. It's not surprising that spinning with an atomic operation doesn't scale very well once a you have more than a handful cores. Of course the whole way locking works in pg_stat_statements is a scalability disaster, but that's a bigger project to fix. Out of curiosity, have you measured whether this has a positive effect without pg_stat_statements? I think it'll e.g. also affect lwlocks, as they also use perform_spin_delay(). Greetings, Andres
