perf(threading): reduce MCS lock handoff latency - #12417
Conversation
The queue head now keeps spinning instead of parking - it is next to acquire, so parking only added wake latency to every handoff - and waiters move through an atomic Waiting -> Parked -> ReadyToAcquire state machine whose store-fence- load pairing closes the missed-wakeup window between the spin phase and the park. Depth-1 handoff throughput roughly doubles in the stress tests. Reentrant acquisition now throws instead of deadlocking silently. ClockCache keeps this improved McsLock: a .NET 10 System.Threading.Lock swap was measured on the reproducible benchmarks and rejected - after the sender cache moved off ClockCache its remaining users (TxPool account cache, chain-level maintenance) are low-contention background work where bounded spinning followed by parking is the right CPU-isolation policy.
|
Claude finished @benaadams's task in 4m 34s —— View job Review: PR #12417 — reduce MCS lock handoff latency
Solid, well-reasoned change. I walked the full Correctness — the handoff/park state machine ✅The core store-fence-load pairing is sound:
I checked the three interleavings (waiter-parks-first, releaser-exchanges-first, and the interleaved case where the releaser blocks on The Memory ordering ✅Dropping the two Consistency of the
|
| Severity | Finding |
|---|---|
| Low | Reentrant acquire now throws InvalidOperationException instead of self-deadlocking — a caller-visible contract change (see inline note on :58). Strictly an improvement, but worth a one-line mention since the PR is tagged only "Optimization". Confirm no call site nests the same lock on one thread. |
| Nit | CurrentThreadNodeForTesting adds a small internal-only test seam; acceptable given it's internal and there was no cleaner way to observe Parked. |
No Critical/High/Medium issues. Nice work — the doubled depth-1 handoff throughput comes from a genuinely tighter, still-correct protocol.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Optimizes McsLock handoff behavior to reduce contended lock latency by keeping the queue head in a bounded yield/spin phase (instead of parking), adding an explicit waiter state machine to close missed-wakeup windows, and failing fast on reentrant acquisition to avoid self-deadlocks.
Changes:
- Update
McsLockto use aWaiting -> Parked -> ReadyToAcquirestate machine with bounded head-of-queue yielding and conditional pulsing of parked successors. - Add reentrancy detection (throw on reentrant acquire) and expose a test hook for observing the current thread’s queue node.
- Refresh
ClockCachelock usage and extend/rename the test suite to validate the new lock semantics and stress behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Nethermind/Nethermind.Core/Threading/MockLock.cs | Renames the scope-entry API to match the updated lock usage pattern. |
| src/Nethermind/Nethermind.Core/Threading/McsLock.cs | Implements the new head-spin + park state machine and reentrancy failure behavior. |
| src/Nethermind/Nethermind.Core/Caching/ClockCache.cs | Switches to the updated lock scope-entry call site (EnterScope). |
| src/Nethermind/Nethermind.Core.Test/MCSLockTests.cs | Removes the prior test file (case/rename and replacement). |
| src/Nethermind/Nethermind.Core.Test/McsLockTests.cs | Adds expanded tests for reentrancy, parked handoff observation, and mixed spin/park stress. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
Changes
Split from #12396 (measurements there).
Waiting -> Parked -> ReadyToAcquirestate machine; the store-fence-load pairing closes the missed-wakeup window between the spin phase and the park.ClockCachedeliberately keeps this improvedMcsLock: a .NET 10System.Threading.Lockswap was measured on the reproducible benchmarks and rejected. After the sender cache moved toAssociativeCache(separate PR), the remainingClockCacheusers (TxPool account cache, chain-level maintenance) are low-contention background work where bounded-spin-then-park is the right CPU-isolation policy (measured wait after this change: ~nil).Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
McsLockTestsextended: reentrancy throw, parked-waiter handoff observation (via the test-only current-node hook), mixed spin/park stress, and the pre-existing suite; ClockCache suite unchanged and green.Documentation
Requires documentation update
Requires explanation in Release Notes