Uh oh!
There was an error while loading. Please reload this page.
threading: lock-free fast path for SemaphoreSlim.WaitAsync - #125452
threading: lock-free fast path for SemaphoreSlim.WaitAsync#125452thomhurst wants to merge 20 commits into
Conversation
thomhurst
commented
Mar 11, 2026
@EgorBot -intel -amd -arm usingSystem.Threading;usingSystem.Threading.Tasks;usingBenchmarkDotNet.Attributes;[MemoryDiagnoser]publicclassSemaphoreSlimUncontended{privateSemaphoreSlim_sem=newSemaphoreSlim(1,1);[Benchmark]publicasyncTaskWaitAsync_Release(){await_sem.WaitAsync();_sem.Release();}} |
There was a problem hiding this comment.
Pull request overview
This PR introduces a lock-free fast path in SemaphoreSlim.WaitAsync that attempts to acquire an available permit via CAS, avoiding taking m_lockObjAndDisposed when uncontended.
Changes:
- Added a CAS-based fast path to decrement
m_currentCountwhen a permit appears immediately available. - Added special-case handling to keep
AvailableWaitHandlestate consistent if it’s initialized concurrently during the fast-path acquire.
Uh oh!
There was an error while loading. Please reload this page.
EgorBo
commented
Mar 11, 2026
Doesn't lock itself has fast paths for that? |
Uh oh!
There was an error while loading. Please reload this page.
thomhurst
commented
Mar 11, 2026
@EgorBot -intel -amd -arm usingSystem.Threading;usingSystem.Threading.Tasks;usingBenchmarkDotNet.Attributes;[MemoryDiagnoser]publicclassSemaphoreSlimUncontended{privateSemaphoreSlim_sem=newSemaphoreSlim(1,1);[Benchmark]publicasyncTaskWaitAsync_Release(){await_sem.WaitAsync();_sem.Release();}} |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
thomhurst
commented
Mar 11, 2026
@EgorBo I think just by not entering the lock we can save some time: EgorBot/Benchmarks#31 |
fb8aeb0 to
419dbb0CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
thomhurst
commented
Apr 4, 2026
@EgorBot -intel -amd -arm usingSystem.Threading;usingSystem.Threading.Tasks;usingBenchmarkDotNet.Attributes;[MemoryDiagnoser]publicclassSemaphoreSlimUncontended{privateSemaphoreSlim_sem=newSemaphoreSlim(1,1);[Benchmark]publicasyncTaskWaitAsync_Release(){await_sem.WaitAsync();_sem.Release();}} |
| int delta = releaseCount - asyncReleased; | ||
| int newCount = delta != 0 ? Interlocked.Add(ref m_currentCount, delta) : observed; | ||
| // atomically, at the end. Whenever waiters are present the count is 0 and no fast path can be | ||
| // racing (it requires count > 0 and no waiters), so this snapshot is stable here. |
| // Fast path: try a lock-free acquire; falls through to the lock if it fails. | ||
| // Skipped when m_waitHandle is non-null to keep its state consistent under the lock. | ||
| if (m_waitHandle is null) |
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "588adeca07fa5f30263b138227a979428b9ca6ea",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "aeac9fb6012b0108ab475fa0acf5d3c757fa26b1",
"last_reviewed_commit": "588adeca07fa5f30263b138227a979428b9ca6ea",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "aeac9fb6012b0108ab475fa0acf5d3c757fa26b1",
"last_recorded_worker_run_id": "29679148684",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "588adeca07fa5f30263b138227a979428b9ca6ea",
"review_id": 4730527049
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: SemaphoreSlim.WaitAsync always enters a Monitor lock even in the common uncontended case where a permit is immediately available. The PR aims to skip that lock via a lock-free CAS fast path, improving uncontended WaitAsync throughput. The motivation is clear, there is a benchmark in the PR discussion, and uncontended async acquire/release is a genuinely hot pattern.
Approach: A fast path in WaitAsyncCore reads m_currentCount and, when a permit looks available with no waiters (m_asyncHead is null && m_waitCount == 0), CAS-decrements the count without taking the lock. To make this safe, the change: makes m_waitCount and m_asyncHead volatile; introduces TryDecrementCount() (a CAS loop) used by every count-decrement site so lock-holders and the lock-free path cannot corrupt the count; converts WaitCore's acquire logic into a retry loop; reworks Release to apply a net delta via Interlocked.Add (never publishing an inflated count that the fast path could steal from); and reworks AvailableWaitHandle lazy init to publish the handle unsignaled behind a MemoryBarrier and gate the fast path on m_waitHandle is null. The reasoning is carefully documented in comments and backed by several new multithreaded stress tests.
The design appears internally consistent: fairness for both synchronous waiters (m_waitCount > 0 deferral) and queued async waiters (m_asyncHead is null gate) is preserved; the async-waiters-imply-count-zero invariant makes the Release snapshot stable when waiters are present; and the store-load fence in AvailableWaitHandle addresses the volatile-write/volatile-read reordering that would otherwise leak a Set handle at count 0.
Summary: SemaphoreSlim — one of the most widely depended-on synchronization primitives in the framework — and the correctness argument rests on delicate lock-free/weak-memory reasoning that cannot be fully validated by reading or by stress tests (which can only fail to disprove rare races, and typically run on strongly-ordered x64). A threading/runtime maintainer with memory-model expertise should scrutinize the ordering claims (especially the AvailableWaitHandle publish barrier, the m_waitCount publication race that motivates the WaitCore retry loop, and the Release net-delta accounting on weakly-ordered arm64) and weigh the added complexity in the contended path (CAS loops replacing plain decrements) against the uncontended-only benefit. Please ensure arm64 CI and the outerloop threading stress suites run.
Detailed Findings
⚠️ Concurrency / Memory Model — correctness rests on unverifiable weak-memory reasoning
The safety of the fast path depends on several subtle claims that hold under the described sequential reasoning but are exactly the class of issue that hides on weakly-ordered hardware and under the JIT's freedom to reorder non-volatile accesses:
- The
AvailableWaitHandlelazy-init ordering: unsignaled publish ofm_waitHandle, thenInterlocked.MemoryBarrier(), then them_currentCountread, paired with the fast path's post-CAS recovery branch (current == 1 && m_waitHandle is not null). This is a classic store-load ordering problem and the correctness window is narrow. - The
Releasenet-delta model relies on the invariant that whenever async waiters are presentm_currentCount == 0and no fast path can race, so theobservedsnapshot is stable. This invariant looks sound but is load-bearing. - The
WaitCoreretry loop is justified by a residual race duringm_waitCount's publication — i.e. the fast path can transiently observem_waitCount == 0for a synchronous waiter that has taken the lock but not yet published. This is correct in spirit, but confirms the fast path and the lock are only loosely coupled.
These are not identified defects — they are the points a human expert should independently verify. Recommend explicit arm64 stress validation before merge.
💡 Performance — contended-path cost of the fast path
The change replaces plain --m_currentCount decrements with TryDecrementCount() CAS loops at all decrement sites and adds volatile to m_asyncHead/m_waitCount. This shifts some cost onto the contended and synchronous paths to benefit the uncontended async path. That tradeoff is probably fine given uncontended is the common case, but the benchmark should ideally also confirm no regression under contention and for synchronous Wait.
✅ Tests — good stress coverage
The new tests target the specific races introduced (concurrent AvailableWaitHandle init, fast-path underflow, WaitCore CAS race, bulk Release handoff, async-waiter handoff racing the fast path, and cancellation racing acquire), gate on IsMultithreadingSupported, and follow existing conventions in the file. This is the right kind of coverage; just note stress tests can only reduce, not eliminate, confidence in rare-race correctness.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 110.3 AIC · ⌖ 10.5 AIC · ⊞ 10K
Use Interlocked.Add to apply a relative delta to m_currentCount rather than writing back an absolute snapshot-derived value, so concurrent lock-free decrements from the WaitAsync fast path are not overwritten.
Replace plain --m_currentCount with a CAS loop to prevent a double grant when the lock-free WaitAsync fast path decrements m_currentCount between the > 0 check and the decrement in the slow path. WaitCore is safe because m_waitCount++ on lock entry blocks the CAS guard for its entire critical section. WaitAsyncCore has no such protection.
Apply the same CAS-loop pattern to WaitCore's m_currentCount decrement that was applied to WaitAsyncCore in the previous commit. A fast-path thread that read m_waitCount = 0 before WaitCore's m_waitCount++ can still race with WaitCore's check-at-404 / decrement-at-407 sequence. The CAS loop serializes both operations on m_currentCount atomically.
… stress test The assert !waitSuccessful || m_currentCount > 0 in WaitCore could fire spuriously in Debug builds: the lock-free WaitAsync fast path runs outside the lock, so it can decrement m_currentCount to 0 between WaitUntilCountOrTimeout returning and the assert executing. Adds a stress test that races AvailableWaitHandle lazy initialization against WaitAsync fast-path acquires and verifies the handle is never signaled when CurrentCount == 0.
…t WaitAsync fast path
…phoreSlim Also fix CS0420 in Release(): Volatile.Read(ref volatile_field) triggers a compiler error in the coreclr project build; replaced with a plain field read (already volatile) so the testhost can be rebuilt with the fixed implementation.
…cessor task is always cancelled
- Extract duplicated CAS-decrement loop into TryDecrementCount() with AggressiveInlining, replacing inline copies in WaitCore and WaitAsyncCore - Strengthen Assert.InRange to Assert.Equal in NeverUnderflows test - Add bulk Release(2) concurrent stress test for Interlocked.Add delta math - Add cancellation-during-fast-path stress test for count integrity - Use m_currentCount (post-Add) instead of netCount for m_waitHandle.Set() - Add UncontendedSync and MixedSyncAsync benchmarks for sync path coverage
Prevents the background task from pegging a CPU core in CI while still exercising concurrent lazy initialization of the wait handle.
… file WaitAsync(CancellationToken) returns Task, not Task<bool>; the prior declaration didn't compile. Removed PerformanceTests/SemaphoreSlimBenchmarks.cs since it had no csproj and wasn't wired into any project; runtime perf benchmarks live in dotnet/performance, and the EgorBot inline benchmarks posted on the PR cover the relevant scenarios.
The comment narrated what the next several lines do; the variable name and surrounding structure already convey it.
Address review hazards in the WaitAsync CAS fast path: - Make m_waitCount and m_asyncHead volatile. The fast path reads them without the lock; sync-waiter writes inside the lock must publish via release semantics rather than depending on the lock release that the fast path bypasses. Without this, ARM64 can let the fast path observe m_waitCount == 0 while a sync waiter is parked, stealing the slot and leaving the waiter blocked. - Restructure AvailableWaitHandle init to publish-then-reflect: publish the handle unsignaled, full barrier, then conditionally Set based on the post-publish count read. Closes the race where ManualResetEvent allocation overlapped a fast-path CAS, leaving the handle Set with count == 0. - WaitCore: loop instead of falling through with a stale waitSuccessful when TryDecrementCount loses to a fast-path acquirer. Fixes the case where Wait(Infinite) could return without owning a permit (silently dropped by the void overload, lying about acquisition for bool overloads). - Release: use Interlocked.Add's return value for the MRE.Set sentinel so a fast-path decrement racing between the Add and the re-read doesn't mask the 0 -> positive transition. - Strengthen the AvailableWaitHandle init test: allocate a fresh SemaphoreSlim per iteration so each iteration is a real attempt at the race. With a single semaphore the race only fires on the first AvailableWaitHandle access.
Defensive: the prior placement was correct because a thrown OCE always short-circuits before re-entry, but moving 'oce' (and 'timedOut', already loop-local) inside makes the freshness invariant explicit and robust to future edits.
Original test had 8 workers each doing WaitAsync;WaitAsync;Release(2) against 4 permits. Under contention, 4 workers could each grab one permit then block on the second WaitAsync — no holder ever reaches Release(2). Helix CI hit this on loaded ARM/Mono interp legs and killed the work item at the 45-min timeout. Split into pure consumers (WaitAsync only) and a single producer issuing Release(2) bulk releases. Same intent — bulk Release racing concurrent fast-path waiters — but deadlock-free by construction.
… race test, dispose test semaphores - Release: replace the single m_currentCount snapshot + max-count check with a CAS loop that re-observes the live count, validates releaseCount against m_maxCount, and applies the increment atomically. Makes the max-count check linearizable with the increment so a concurrent lock-free WaitAsync fast-path decrement can no longer trigger a spurious SemaphoreFullException. - Tests: rewrite WaitAsync_CancellationDuringFastPath_NoCountCorruption (which never exercised the race, since an uncontended WaitAsync on (1,1) completes synchronously before Cancel) as WaitAsync_CancellationRacingAcquire_NoCountCorruption: contended workers with each token cancelled from a separate thread. - Tests: dispose the SemaphoreSlim instances in the new tests via using.
The handoff loop tracked two opposing counters (decrementing maxAsyncToRelease and incrementing asyncReleased) for the same loop progress. Use a single up-counter compared against the fixed bound instead. No behavior change.
Release handed permits to async waiters by CAS-bumping m_currentCount to observed + releaseCount, draining waiters from the list, then subtracting the handed-out count with Interlocked.Add. Between removing a waiter from the list and the subtract, m_currentCount was inflated while m_asyncHead and m_waitCount read as empty, so the lock-free WaitAsync fast path could acquire a permit already earmarked for that waiter. This double-acquired a permit and drove the count negative, after which a later Release exceeded m_maxCount and threw SemaphoreFullException. The weaker Mono memory model exposed it (Mono_MiniJIT libraries tests crashing in xUnit's WaitAsync throttle), but the race is platform-independent. Never publish the inflated count. Compute the post-release count in a local, drain waiters, and apply only the net delta (releaseCount - asyncReleased) in a single Interlocked.Add at the end, so the fast path never observes a permit reserved for a waiter. The max-count check is linearized before any waiter is touched by re-reading the live count on a mismatch, keeping the spurious-throw protection without the bump. Add Release_AsyncWaiterHandoff_RacingFastPath_NeverExceedsMaxCount to cover the async-waiter handoff racing the fast path. Also switch the cancellation race test's per-iteration canceller from Task.Run to ThreadPool to drop the per-iteration Task allocation, and fix a grammar typo in a WaitCore comment.
588adec to
1cfd952Compare
Use a lock-free CAS fast path in SemaphoreSlim.WaitAsync to skip the Monitor lock when a permit is immediately available, improving uncontended throughput