From 3fc03d842c45d9742793797096d60dcbebd7c399 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Wed, 7 Oct 2026 00:42:43 -0700 Subject: [PATCH 1/4] perf(thread.h): faster spin_mutex under contention; fix arm64 pause * 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 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::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 --- src/include/OpenImageIO/thread.h | 69 +++++++++++++++++--------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/src/include/OpenImageIO/thread.h b/src/include/OpenImageIO/thread.h index 7d5ceaaefb..ab7cc7cdbb 100644 --- a/src/include/OpenImageIO/thread.h +++ b/src/include/OpenImageIO/thread.h @@ -134,14 +134,25 @@ yield() noexcept -// Slight pause +// Slight pause. Unrolling a constant-delay pause bloats every inlined +// spin_mutex::lock(), so tell the compiler not to. +#define OIIO_PAUSE_NOUNROLL \ + OIIO_CLANG_PRAGMA(nounroll) OIIO_GCC_ONLY_PRAGMA(GCC unroll 1) + inline void pause(int delay) noexcept { #if defined(__GNUC__) && (defined(__x86_64__) || defined(__i386__)) + OIIO_PAUSE_NOUNROLL for (int i = 0; i < delay; ++i) __asm__ __volatile__("pause;"); +#elif defined(__GNUC__) && defined(__aarch64__) + // isb, not yield: on Apple Silicon, yield is effectively a nop. + OIIO_PAUSE_NOUNROLL + for (int i = 0; i < delay; ++i) + __asm__ __volatile__("isb" ::: "memory"); + #elif defined(__GNUC__) && (defined(__arm__) || defined(__s390__)) for (int i = 0; i < delay; ++i) __asm__ __volatile__("NOP;"); @@ -173,10 +184,14 @@ pause(int delay) noexcept // Helper class to deliver ever longer pauses until we yield our timeslice. +// Pauses double from 1 up to `pausemax`, then repeat `pausemax` for +// `spins` more rounds, then each call yields the timeslice. class atomic_backoff { public: - atomic_backoff(int pausemax = 16) noexcept - : m_count(1), m_pausemax(pausemax) + atomic_backoff(int pausemax = 16, int spins = 0) noexcept + : m_count(1) + , m_pausemax(pausemax) + , m_spins(spins) { } @@ -185,6 +200,9 @@ class atomic_backoff { if (m_count <= m_pausemax) { pause(m_count); m_count *= 2; + } else if (m_spins > 0) { + pause(m_pausemax); + --m_spins; } else { std::this_thread::yield(); } @@ -193,6 +211,7 @@ class atomic_backoff { private: int m_count; int m_pausemax; + int m_spins; }; @@ -234,34 +253,19 @@ class spin_mutex { /// void lock() noexcept { - // To avoid spinning too tightly, we use the atomic_backoff to - // provide increasingly longer pauses, and if the lock is under - // lots of contention, eventually yield the timeslice. - atomic_backoff backoff; - - // Try to get ownership of the lock. Though experimentation, we - // found that OIIO_UNLIKELY makes this just a bit faster on gcc - // x86/x86_64 systems. - while (!OIIO_UNLIKELY(try_lock())) { -#if OIIO_THREAD_ALLOW_DCLP - // The full try_lock() involves a test_and_set, which - // writes memory, and that will lock the bus. But a normal - // read of m_locked will let us spin until the value - // changes, without locking the bus. So it's faster to - // check in this manner until the mutex appears to be free. - // HOWEVER... Thread Sanitizer things this is an instance of - // an unsafe "double checked lock pattern" (DCLP) and flags it - // as an error. I think it's a false negative, because the - // outer loop is still an atomic check, the inner non-atomic - // loop only serves to delay, and can't lead to a true data - // race. But we provide this build-time switch to, at least, - // give a way to use tsan for other checks. + // Back off with increasingly longer pauses. Spin a long while + // before yielding the timeslice: yielding early is what hurts + // most under contention. + atomic_backoff backoff(128, 64); + + // Try to get ownership of the lock. Failing is the unlikely case. + while (OIIO_UNLIKELY(!try_lock())) { + // The exchange in try_lock() writes the cache line. Spin on a + // plain relaxed load until the lock looks free, so waiters + // don't keep stealing the line from the holder. do { backoff(); - } while (*(volatile bool*)&m_locked); -#else - backoff(); -#endif + } while (m_locked.load(std::memory_order_relaxed)); } } @@ -270,8 +274,7 @@ class spin_mutex { void unlock() noexcept { OIIO_TSAN_PRE_UNLOCK(&m_locked, 0); - // Fastest way to do it is with a clear with "release" semantics - m_locked.clear(std::memory_order_release); + m_locked.store(false, std::memory_order_release); OIIO_TSAN_POST_UNLOCK(&m_locked, 0); } @@ -280,7 +283,7 @@ class spin_mutex { bool try_lock() noexcept { OIIO_TSAN_PRE_LOCK(&m_locked, OIIO_TSAN_F_TRY); - const bool got = !m_locked.test_and_set(std::memory_order_acquire); + const bool got = !m_locked.exchange(true, std::memory_order_acquire); OIIO_TSAN_POST_LOCK(&m_locked, got ? OIIO_TSAN_F_TRY : OIIO_TSAN_F_TRY_FAILED); return got; @@ -301,7 +304,7 @@ class spin_mutex { }; private: - std::atomic_flag m_locked = ATOMIC_FLAG_INIT; // initialize to unlocked + std::atomic m_locked { false }; }; From eae5b61894fec693e2921c63978e6cdf60013913 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Mon, 12 Oct 2026 18:38:37 -0700 Subject: [PATCH 2/4] fomat Signed-off-by: Larry Gritz --- src/include/OpenImageIO/thread.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/include/OpenImageIO/thread.h b/src/include/OpenImageIO/thread.h index ab7cc7cdbb..4643c953ed 100644 --- a/src/include/OpenImageIO/thread.h +++ b/src/include/OpenImageIO/thread.h @@ -189,9 +189,7 @@ pause(int delay) noexcept class atomic_backoff { public: atomic_backoff(int pausemax = 16, int spins = 0) noexcept - : m_count(1) - , m_pausemax(pausemax) - , m_spins(spins) + : m_count(1), m_pausemax(pausemax), m_spins(spins) { } From 56ed74fcb3675943e176fe775e3b3fa33424f36e Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Fri, 16 Oct 2026 17:28:00 -0700 Subject: [PATCH 3/4] Update src/include/OpenImageIO/thread.h Co-authored-by: Jinnie Kim Signed-off-by: Larry Gritz --- src/include/OpenImageIO/thread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/OpenImageIO/thread.h b/src/include/OpenImageIO/thread.h index 4643c953ed..c259b93311 100644 --- a/src/include/OpenImageIO/thread.h +++ b/src/include/OpenImageIO/thread.h @@ -148,7 +148,7 @@ pause(int delay) noexcept __asm__ __volatile__("pause;"); #elif defined(__GNUC__) && defined(__aarch64__) - // isb, not yield: on Apple Silicon, yield is effectively a nop. + // isb, not yield: yield is effectively a nop (e.g. on Apple Silicon). OIIO_PAUSE_NOUNROLL for (int i = 0; i < delay; ++i) __asm__ __volatile__("isb" ::: "memory"); From 0ec2704bf6bfee52368c5a06595d7a8e5507b4e2 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Fri, 16 Oct 2026 18:05:02 -0700 Subject: [PATCH 4/4] Rewrite pause loop to apply OIIO_PAUSE_NOUNROLL to all platforms Signed-off-by: Larry Gritz --- src/include/OpenImageIO/thread.h | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/include/OpenImageIO/thread.h b/src/include/OpenImageIO/thread.h index c259b93311..c878c8a3ad 100644 --- a/src/include/OpenImageIO/thread.h +++ b/src/include/OpenImageIO/thread.h @@ -142,23 +142,16 @@ yield() noexcept inline void pause(int delay) noexcept { -#if defined(__GNUC__) && (defined(__x86_64__) || defined(__i386__)) OIIO_PAUSE_NOUNROLL - for (int i = 0; i < delay; ++i) + for (int i = 0; i < delay; ++i) { +#if defined(__GNUC__) && (defined(__x86_64__) || defined(__i386__)) __asm__ __volatile__("pause;"); - #elif defined(__GNUC__) && defined(__aarch64__) - // isb, not yield: yield is effectively a nop (e.g. on Apple Silicon). - OIIO_PAUSE_NOUNROLL - for (int i = 0; i < delay; ++i) + // isb, not yield: yield is effectively a nop (e.g. on Apple Silicon). __asm__ __volatile__("isb" ::: "memory"); - #elif defined(__GNUC__) && (defined(__arm__) || defined(__s390__)) - for (int i = 0; i < delay; ++i) __asm__ __volatile__("NOP;"); - #elif defined(_MSC_VER) - for (int i = 0; i < delay; ++i) { // A reimplementation of winnt.h YieldProcessor, // to avoid including windows headers. # if defined(_M_AMD64) @@ -172,13 +165,10 @@ pause(int delay) noexcept # else _asm pause # endif - } - #else - // No pause on this platform, just punt - for (int i = 0; i < delay; ++i) - ; + // No pause on this platform, just punt #endif + } }