[API] Back SpinLockMutex with SRWLOCK on Windows to fix tail latency#4245
[API] Back SpinLockMutex with SRWLOCK on Windows to fix tail latency#4245abhinandan-codes wants to merge 2 commits into
Conversation
On Windows the SpinLockMutex back-off falls through to std::this_thread::sleep_for(1ms). Windows rounds sleep durations up to the system timer resolution (~15.6ms by default), so a thread that loses the lock race parks for a full timer quantum instead of ~1ms. Under contention on a single instrument (e.g. a hot histogram in the metrics SDK) this produces a large tail latency (p99 ~15ms) even though the median is a few microseconds. Back the mutex with a Slim Reader/Writer (SRW) lock on _MSC_VER. SRWLOCK spins briefly then parks the waiter on a kernel keyed event and wakes it immediately on release, independent of the timer resolution and without busy-spinning. It is pointer-sized, non-recursive, and keeps the same BasicLockable, non-copyable and non-movable contract. Other platforms are unchanged. Signed-off-by: Abhinandan Sharma <abhinandans@microsoft.com>
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4245 +/- ##
==========================================
+ Coverage 77.92% 77.93% +0.02%
==========================================
Files 439 439
Lines 18612 18612
==========================================
+ Hits 14502 14504 +2
+ Misses 4110 4108 -2
🚀 New features to boost your workflow:
|
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR and for pointing out the 1ms sleep issue. Please see some initial feedback and question below.
| void unlock() noexcept { ReleaseSRWLockExclusive(&mutex_); } | ||
|
|
||
| private: | ||
| SRWLOCK mutex_ = SRWLOCK_INIT; |
There was a problem hiding this comment.
This will be an ABI break.
Please see
| return; | ||
| } | ||
| // Sleep and then start the whole process again. (goal ~1000ns) | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(SPINLOCK_SLEEP_MS)); |
There was a problem hiding this comment.
The 1ms sleep (which may sleep longer) seems undesirable for any platform. Is there a common approach we can apply that is platform agnostic and doesn't break ABI for the class?
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
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.
|
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 |
5 participants
Problem
On Windows,
SpinLockMutex::lock()falls through tostd::this_thread::sleep_for(std::chrono::milliseconds(1))as its finalback-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-streamSpinLockMutex), this produces a large tail latency: the medianRecord()is afew microseconds but p99 jumps to ~15 ms.
Root cause
sleep_for(1ms)is not 1 ms on Windows. The intended sub-millisecondincremental back-off becomes a ~15.6 ms stall whenever a waiter reaches the
sleep phase.
Fix
Back
SpinLockMutexwith a Slim Reader/Writer (SRW) lock on_MSC_VER.SRWLOCKspins briefly, then parks the waiter on a kernel keyed event and wakesit immediately on release — independent of the timer resolution and with no
busy-spin. It is pointer-sized, non-recursive, and preserves the existing
BasicLockable/ non-copyable / non-movable contract. Non-Windows platformsare unchanged.
Measurements
Windows 11, 6 threads hammering one histogram at ~2000 rec/s, default 15.6 ms
timer:
sleep_for)Notes
surface).
timeBeginPeriod(1)mitigates the symptom globally but is a process-wide sideeffect, not a library-level fix.