[API] SpinLockMutex contention stalls a full timer quantum (~15.6 ms) on Windows

[abhinandan-codes]

Problem

On Windows, SpinLockMutex::lock() falls through to
std::this_thread::sleep_for(std::chrono::milliseconds(1)) as its final
back-off step. Windows rounds sleep durations up to the current system timer
resolution, which defaults to ~15.6 ms. So a thread that loses the lock race
does not sleep ~1 ms — it parks for a full timer quantum (~15.6 ms).

Under sustained contention on a single instrument (for example a hot histogram
recorded from many threads via SyncMetricStorage, which holds a per-stream
SpinLockMutex), this produces a large tail latency: the median Record() is a
few microseconds but p99 jumps to ~15 ms.

Root cause

sleep_for(1ms) is not 1 ms on Windows. The intended sub-millisecond
incremental back-off becomes a ~15.6 ms stall whenever a waiter reaches the
sleep phase.

Fix

Exploring options — see comment below for the two approaches and benchmarks; open to maintainer preference.

Measurements

Windows 11, 6 threads hammering one histogram at ~2000 rec/s, default 15.6 ms
timer:

build	p50	p99	records >= 5 ms
SpinLock (sleep_for)	few us	~15 ms	~52%

Notes

• Header-only change in the API; no ABI break (same size class, same public
  surface).
• timeBeginPeriod(1) mitigates the symptom globally but is a process-wide side
  effect, not a library-level fix.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: abhinandan-codes / name: Abhinandan Sharma (b0047b2)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.07%. Comparing base (5d7db84) to head (357bf71).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4245      +/-   ##
==========================================
- Coverage   81.15%   81.07%   -0.07%     
==========================================
  Files         446      446              
  Lines       18922    18922              
==========================================
- Hits        15355    15340      -15     
- Misses       3567     3582      +15     

Files with missing lines	Coverage Δ
...ntelemetry/sdk/metrics/state/sync_metric_storage.h	83.75% <100.00%> (ø)
sdk/src/metrics/state/sync_metric_storage.cc	93.69% <100.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbarker]

Thanks for the PR and for pointing out the 1ms sleep issue. Please see some initial feedback and question below.

[marcalff]

Thanks for the patch.

As described by @dbarker, this change breaks the ABI: it does not "backs" the spin lock with a read write lock, it just replaces the spin lock by a read write lock instead.

Given how multiple libraries can be compiled with different versions of this header file, and how each instrumentation will need a lock to use tracer / meter / logger providers, this is a major issue.

A better way would be to change the implementation of the lock() method alone, when waiting for the spin lock to be available.

[malkia]

SRWLocks are not re-entrant - not sure if we fall into situation where this might happen. This also calls for AppVerif test on Windows through cuzz and other verifiers.

[abhinandan-codes]

Context: reworking this PR from the SRWLOCK approach to a parking back-off, and a question for the SDK team on which direction you'd prefer.

Option 1 — park the waiter in SpinLockMutex (this PR).
Keep the existing atomic spin lock and its fast-spin/yield phases; only replace the final sleep_for(1ms) with parking the waiter, woken immediately on unlock():

• C++20: std::atomic<bool>::wait() / notify_one().
• Pre-C++20 MSVC (Win8+): WaitOnAddress / WakeByAddressSingle.
• Otherwise: the previous sleep_for fallback (unchanged off-Windows behavior).

The std::atomic<bool> member is unchanged, so the public API layout/ABI is preserved. It's a header-only change and fixes the lock for every SpinLockMutex user (trace/logs/metrics), not just metrics.

Option 2 — an SDK-local, CMake-selectable lock for the metrics hot path.
Leave the API SpinLockMutex untouched and introduce sdk::common::SyncLock, selected at build time (OPENTELEMETRY_SDK_METRICS_LOCK = spinlock default / srwlock / std_mutex), swapping the metrics-SDK lock callsites. The default build is a no-op; a non-default value changes SDK metrics class layout, so the macro must be set consistently across the SDK build and consumers (propagated as a PUBLIC/INTERFACE compile definition). This keeps the change opt-in and confined to metrics, at the cost of an ABI/build-consistency contract.

Separately wanted to know your testing infrastructure setup as option 1 may need testing it across C++ versions and platforms

Which path would you prefer — parking the API SpinLockMutex (Option 1), the CMake-selectable SDK lock (Option 2), or another approach you have in mind? Happy to adjust this PR accordingly. This is a critical bug blocking our move to OpenTelemetry, so any steer on the preferred direction would be a big help. Thanks!

Patch changed, need to review new code

[lalitb]

@abhinandan-codes - The Windows issue is real, and replacing the 1 ms sleep with parking is a good idea. My concern is that the measured problem is in the metrics hot path, but this PR changes the shared SpinLockMutex used across metrics, tracing, logging, providers, and other platforms.

Because this lock is defined in a header, lock() and unlock() now get different definitions depending on the C++ standard and _WIN32_WINNT. If linked code uses different settings, that violates the ODR. Some code may park and expect a notification, while other code may only clear the flag without notifying. The program may still link, but a waiting thread could get stuck. Keeping the same class size avoids a layout ABI break, but it does not make these implementations compatible.

There is also a concrete compatibility issue: a C++20 build targeting macOS 10.15 can select atomic::wait(), while libc++ supports that operation only from macOS 11.

The change also performs a notification on every unlock, including uncontended paths outside metrics, but the PR only provides measurements for the Windows metrics case. There are no correctness tests for multiple waiters or repeated lock handoff either.

I suggest keeping common::SpinLockMutex unchanged and using a fixed SDK-internal lock for the affected metrics path. A std::mutex, which uses an efficient Windows lock underneath, or a dedicated internal parking lock would solve the measured problem without changing synchronization behavior across the whole project. I would also avoid a CMake option that changes the lock type, because that can create another mixed-layout problem.

Overall: the problem and general direction are good, but I think the current implementation is too broad and has compatibility risks. I’m requesting changes and would prefer the SDK-internal metrics approach.

[lalitb]

As discussed here

[marcalff]

I suggest keeping common::SpinLockMutex unchanged and using a fixed SDK-internal lock for the affected metrics path. A std::mutex, which uses an efficient Windows lock underneath, or a dedicated internal parking lock would solve the measured problem without changing synchronization behavior across the whole project. I would also avoid a CMake option that changes the lock type, because that can create another mixed-layout problem.

Overall: the problem and general direction are good, but I think the current implementation is too broad and has compatibility risks. I’m requesting changes and would prefer the SDK-internal metrics approach.

Thanks @lalitb for the in depth analysis.

I agree with lalit here, using a different lock instead of a spin lock where there is contention for metrics is less intrusive compared to changing the spin lock implementation itself for every uses.

A spin lock is supposed to be held for a very short time, not causing contention. If there is contention, it could be because the lock is held for too long, so that a spin lock is not appropriate in the user case found with metrics.

[ThomsonTan]

I suggest keeping common::SpinLockMutex unchanged and using a fixed SDK-internal lock for the affected metrics path. A std::mutex, which uses an efficient Windows lock underneath, or a dedicated internal parking lock would solve the measured problem without changing synchronization behavior across the whole project. I would also avoid a CMake option that changes the lock type, because that can create another mixed-layout problem.
Overall: the problem and general direction are good, but I think the current implementation is too broad and has compatibility risks. I’m requesting changes and would prefer the SDK-internal metrics approach.

Thanks @lalitb for the in depth analysis.

I agree with lalit here, using a different lock instead of a spin lock where there is contention for metrics is less intrusive compared to changing the spin lock implementation itself for every uses.

A spin lock is supposed to be held for a very short time, not causing contention. If there is contention, it could be because the lock is held for too long, so that a spin lock is not appropriate in the user case found with metrics.

I think we can scope it down to a single member. The measured hot path is entirely behind one lock:

opentelemetry-cpp/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h

Line 303 in 5d7db84

opentelemetry::common::SpinLockMutex attribute_hashmap_lock_;

All four RecordLong() / RecordDouble() overloads serialize on attribute_hashmap_lock_, so switching just this one member to std::mutex (plus the four lock_guard instantiations) covers the "hot histogram from many threads" case in this thread.

[dbarker]

I suggest keeping common::SpinLockMutex unchanged and using a fixed SDK-internal lock for the affected metrics path. A std::mutex, which uses an efficient Windows lock underneath, or a dedicated internal parking lock would solve the measured problem without changing synchronization behavior across the whole project. I would also avoid a CMake option that changes the lock type, because that can create another mixed-layout problem.
Overall: the problem and general direction are good, but I think the current implementation is too broad and has compatibility risks. I’m requesting changes and would prefer the SDK-internal metrics approach.

Thanks @lalitb for the in depth analysis.
I agree with lalit here, using a different lock instead of a spin lock where there is contention for metrics is less intrusive compared to changing the spin lock implementation itself for every uses.
A spin lock is supposed to be held for a very short time, not causing contention. If there is contention, it could be because the lock is held for too long, so that a spin lock is not appropriate in the user case found with metrics.

I think we can scope it down to a single member. The measured hot path is entirely behind one lock:

opentelemetry-cpp/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h

Line 303 in 5d7db84

opentelemetry::common::SpinLockMutex attribute_hashmap_lock_;

All four RecordLong() / RecordDouble() overloads serialize on attribute_hashmap_lock_, so switching just this one member to std::mutex (plus the four lock_guard instantiations) covers the "hot histogram from many threads" case in this thread.

I agree, the metrics recording path should use a std::mutex. The complexity of the base2 histogram aggregation and last value aggregation (it has a system call to read the clock) may trigger the 1ms sleep of the spinlock.

Please take a look at #4317 which documents the current SpinLockMutex usage and proposes which cases to switch to std::mutex.

[marcalff]

LGTM, thanks for the fix.

[marcalff]

@abhinandan-codes

Thanks for your work and patience on this issue.

Please check CI (format, IWYU) and merge conflicts

[lalitb]

LGTM, Thanks @abhinandan-codes. Check the Ci, and also rebase to resolve the merge conflict.

[dbarker]

Thanks again for highlighting this issue! Switching to std::mutex in the sync metric storage (and elsewhere in the SDK) is necessary for synchronization consistency on all platforms and deployments.