Repository navigation
perf(thread.h): faster spin_mutex under contention; fix arm64 pause - #5564
Conversation
* pause() had no aarch64 branch (only `__arm__`), so on Apple Silicon and Linux arm64 it was an empty loop. Use `isb`; the `yield` hint is effectively a nop on Apple cores and benchmarks badly. Also keep the compiler from fully unrolling a constant pause count, which bloated lock() enough that clang -O3 stopped inlining it. * spin_mutex::lock() hinted the branch backwards (`!OIIO_UNLIKELY(try_lock())`), marking acquisition as the unlikely case. Worth up to 2x at 16-32 threads. * spin_mutex now holds a std::atomic<bool> and waits with a relaxed load instead of reading the atomic_flag through a volatile bool cast (UB, and the reason TSan needed OIIO_THREAD_ALLOW_DCLP=0). Codegen is identical. Note: on MSVC this changes sizeof(spin_mutex) from 4 to 1. * Yielding early was the main cost at 8-12 threads. atomic_backoff gets an optional `spins` count (rounds at the max pause before yielding, default 0 = old behavior, which ImageCache's tile wait still uses). spin_mutex uses atomic_backoff(128, 64), which was the best of the backoff variants benchmarked on both machines below. spinlock_test, old -> new: - M4 Max (16 cores), ns/op: 4 threads 3.8 -> 1.2, 8 threads 26.2 -> 2.5, 12 threads 18.8 -> 3.8, 16 threads 5.0 -> 5.0. Old is faster only when oversubscribed (32 threads 2.5 -> 6.2). - Threadripper PRO 5955WX (16 cores / 32 threads, gcc 11), seconds: 8 threads 3.7 -> 2.3, 16 threads 6.3 -> 4.3, 32 threads 10.9 -> 7.5, 48 threads 10.2 -> 8.1, 64 threads 9.2 -> 9.1; less CPU time up to 48. Uncontended cost is unchanged on both. parallel_test task launch rate is up 5-12% on x86. spin_rw_test and testtex are unchanged. C++20 atomic_flag::test() gives nothing over atomic<bool>::load() (and benchmarked worse on x86), and wait()/notify() is ~4x slower uncontended. Under heavy contention on macOS it also drove __ulock_wait/__ulock_wake into a kernel spinlock timeout panic, so it is deliberately not used. Assisted-by: Claude Code / Claude Opus 5.5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
jinhgkim
left a comment
There was a problem hiding this comment.
Spent most of my time learning the subject... and it's pretty cool! Everything makes sense here. I just left one suggestion and a question.
| for (int i = 0; i < delay; ++i) | ||
| __asm__ __volatile__("isb" ::: "memory"); | ||
|
|
||
| #elif defined(__GNUC__) && (defined(__arm__) || defined(__s390__)) |
There was a problem hiding this comment.
Out of curiosity, if unrolling bloats every inlined lock, why isn't OIIO_PAUSE_NOUNROLL used here too?
There was a problem hiding this comment.
Correct. Refactor of this function coming...
There was a problem hiding this comment.
updated with your suggestion
Co-authored-by: Jinnie Kim <jinhgkim@gmail.com> Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Is this true by the way? The speedup here is crazy. On my machine (M5 Pro chip), it's actually slightly slower (8 threads 10.6 -> 12.6, 12 threads 18.2 -> 19.0), and I don't know why... (I've been having a long chat with Claude but still can't figure it out.) The other thread counts match yours (4 threads 3x faster, 16 the same, 32 ~2.7x slower), so it's just 8 and 12 that differ. |
Do you know what your core counts are? I reran the benchmarks on my M4 Max laptop (12 performance cores and 4 efficiency cores). And got a detailed Claude comparison (it's great at writing a script to kick off a bunch of N-way comparison benchmarks that are more tedious than I would want to do by hand), reproduced below. I also had (not repeated today, but done when I did the original work) ran tests on my work machine (Linux x86_64 64 cores) to verify that nothing bad happened there). The PR is a big win at 2-12 threads on the M4, but it's about 2x slower than before at 16 threads (all cores) in
Why 16T regresses (my guess, not measured): the M4 Max has 12 performance and 4 efficiency cores. If the lock holder lands on a slower efficiency core, the 15 threads spinning on The original M4 study's own benchmark showed 16T improving (21.3 -> 12.6 ns on a tiny critical section). That benchmark had a different shape, and |
6 super cores + 12 performance cores |
|
So I have 12 perf + 4 efficiency, and am ok at <= 12 cores but hit a performance cliff at 16. You have 6 super + 12 perf, and hit a performance cliff at 8 cores? That sounds suspiciously like as soon as the job spills from using only the machine's best core type to being a mix of that and a second core type, things aren't as smooth. I wonder if these Apple computers have some odd bus synchronization properties when you're mixing different core type accessing the same memory locations? |
|
Yesss, mine is fine at <= 6
|
|
Did some experiments mixing core types with the help of Claude
Mixing core types doesn't hurt the PR (14.7 is between all-Super and all-P), and it's still about 2.3x faster than Base. Claude says the PR is slower at 8T and 12T because Base suddenly gets faster when the thread count goes above the best core type's count.
Also a summary of Claude's conclusion: The reason seems to be Base's yields. Base's waiting threads call sched_yield() constantly. Once there are more threads than top-tier cores, some threads have to run on lower-tier cores, off their preferred core type, and that likely makes each yield on the top-tier cores go through the full scheduler to check whether those threads should be moved back (see XNU's I think this makes sense, and it seems to fit the benchmarks on both Macs. What do you think? |
|
Yeah, that makes sense to me. There's a sense in which the benchmark isn't quite measuring the same thing once cores are oversubscribed or when they are using different core types, versus when each thread gets a core and they're all the same kind. So those charts are really mixing two different situations and pretending it's a simple linear relationship. The spin_mutex is meant for situations where contention is reasonably low and the lock is held for a very short time. Cranking up the contention as high as we can make it for the benchmarks -- literally doing nothing outside the critical path, just having all threads hit the lock incessantly -- is pushing them out of the use case they were designed for or that they are likely to encounter in the real world. I think the bottom line is that the bug fixes and the new backoff makes the performance curve much more predictable, and dramatically better when all threads get cores (which is the usual situation, although they are slightly slower than before when in the odd situation of mixed cores or not enough cores) and in the lower-contention domain they were designed for. I think that's a fair tradeoff. |
This started as my taking a look at whether our spin_mutex already used all the good techniques discussed in this article that I happened to read last week: https://david.alvarezrosa.com/posts/optimizing-a-spin-lock/
And indeed, there were some improvements to make, primarily in the way we pause and back off when the lock is held by another thread. But also, some bugs were found along the way. Changes in this PR:
pause() had no aarch64 branch (only
__arm__), so on Apple Silicon and Linux arm64 it was an empty loop. Useisb; theyieldhint is effectively a nop on Apple cores and benchmarks badly. Also keep the compiler from fully unrolling a constant pause count, which bloated lock() enough that clang -O3 stopped inlining it.spin_mutex::lock() hinted the branch backwards (
!OIIO_UNLIKELY(try_lock())), marking acquisition as the unlikely case. It should have readOIIO_UNLIKELY(!try_lock()), a subtle difference, but worth up to 2x at 16-32 threads!spin_mutex now holds a
std::atomic<bool>and waits with a relaxed load instead of reading theatomic_flagthrough a volatile bool cast (UB, and the reason TSan needed OIIO_THREAD_ALLOW_DCLP=0). Codegen is identical. Note: on MSVC this changes sizeof(spin_mutex) from 4 to 1.Yielding early was the main cost at 8-12 threads. atomic_backoff gets an optional
spinscount (rounds at the max pause before yielding, default 0 = old behavior, which ImageCache's tile wait still uses). spin_mutex uses atomic_backoff(128, 64), which was the best of the backoff variants benchmarked on both machines below.spinlock_test, old -> new:
C++20
atomic_flag::test()gives nothing overatomic<bool>::load()(and benchmarked worse on x86), and wait()/notify() is ~4x slower uncontended. Under heavy contention on macOS it also drove __ulock_wait/__ulock_wake into a kernel spinlock timeout panic, so it is deliberately not used.I have only the spin_mutex change in thread.h in this PR. I will have subsequent follow-on work elsewhere (including ImageCache), that makes further improvements that this work later turned into.
Assisted-by: Claude Code / Claude Opus 5.5