From 14a303d0c90d95a2373d08b2219786aa0f981aa5 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Tue, 19 Aug 2025 20:02:13 -0700 Subject: [PATCH 01/13] Enable fast path in Wait/Release for SemaphoreSlim --- .../src/System/Threading/SemaphoreSlim.cs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index a208ee4a1f610c..b429b310cba981 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -336,6 +336,17 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo return false; } + // Perf: If there is no wait handle, we can try entering the semaphore without using the lock. + // Otherwise, we would actually need to take the lock to avoid race conditions when calling m_waitHandle.Reset() + if (m_waitHandle is null) + { + int currentCount = m_currentCount; + if (currentCount > 0 && Interlocked.CompareExchange(ref m_currentCount, currentCount - 1, currentCount) == currentCount) + { + return true; + } + } + long startTime = 0; if (millisecondsTimeout != Timeout.Infinite && millisecondsTimeout > 0) { @@ -848,6 +859,16 @@ public int Release(int releaseCount) throw new ArgumentOutOfRangeException( nameof(releaseCount), releaseCount, SR.SemaphoreSlim_Release_CountWrong); } + + if (m_waitCount == 0 && m_waitHandle is null && m_asyncHead is null) + { + int currentCount = m_currentCount; + if (m_maxCount - currentCount >= releaseCount && Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) + { + return currentCount; + } + } + int returnCount; lock (m_lockObjAndDisposed) From e87080854a56aabe8bcb4c5a5dc4f41ebc04f160 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Thu, 21 Aug 2025 14:30:43 -0700 Subject: [PATCH 02/13] Fix race conditions --- .../src/System/Threading/SemaphoreSlim.cs | 130 +++++++++++++++--- 1 file changed, 114 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index b429b310cba981..13ac51af800e23 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -40,7 +40,7 @@ public class SemaphoreSlim : IDisposable // The number of synchronously waiting threads, it is set to zero in the constructor and increments before blocking the // threading and decrements it back after that. It is used as flag for the release call to know if there are // waiting threads in the monitor or not. - private int m_waitCount; + private volatile int m_waitCount; /// /// This is used to help prevent waking more waiters than necessary. It's not perfect and sometimes more waiters than @@ -57,7 +57,7 @@ public class SemaphoreSlim : IDisposable private volatile ManualResetEvent? m_waitHandle; // Head of list representing asynchronous waits on the semaphore. - private TaskNode? m_asyncHead; + private volatile TaskNode? m_asyncHead; // Tail of list representing asynchronous waits on the semaphore. private TaskNode? m_asyncTail; @@ -65,6 +65,11 @@ public class SemaphoreSlim : IDisposable // No maximum constant private const int NO_MAXIMUM = int.MaxValue; + // Used to track if we are attempting the fast path (without taking the lock). + // Therefore, if another thread takes the lock, shouldn't start its operations until + // all threads finish their attempt to use the fast path. + private volatile int m_threadsTryingFastPathCount; + // Task in a linked list of asynchronous waiters private sealed class TaskNode : Task { @@ -106,6 +111,10 @@ public WaitHandle AvailableWaitHandle // lock the count to avoid multiple threads initializing the handle if it is null lock (m_lockObjAndDisposed) { + if (m_threadsTryingFastPathCount > 0) + { + SpinUntilFastPathFinishes(); + } // The initial state for the wait handle is true if the count is greater than zero // false otherwise m_waitHandle ??= new ManualResetEvent(m_currentCount != 0); @@ -310,6 +319,51 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) return WaitCore(millisecondsTimeout, cancellationToken); } + /// + /// Tries a fast path to wait on the semaphore without taking the lock. + /// + /// true if the fast path succeeded; otherwise, false. + [UnsupportedOSPlatform("browser")] + private bool TryWaitFastPath() + { + bool result = false; + if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null) + { + Interlocked.Increment(ref m_threadsTryingFastPathCount); + // It's possible that the lock is taken by now, don't attempt the fast path in that case. + // However if it's not taken by now, it's safe to attempt the fast path. + // If the lock is taken after checking this condition, because m_threadsTryingFastPathCount is already greater than 0, + // the thread holding the lock wouldn't start its operations. + // If the wait handle is not null, we have to follow the slow path taking the lock to avoid race conditions. + // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, did its work and released it + // before this thread updated m_threadsTryingFastPathCount. + if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null) + { + int currentCount = m_currentCount; + if (currentCount > 0 && + Interlocked.CompareExchange(ref m_currentCount, currentCount - 1, currentCount) == currentCount) + { + result = true; + } + } + Interlocked.Decrement(ref m_threadsTryingFastPathCount); + } + return result; + } + + /// + /// Blocks the current thread until all the threads trying the fast path finish. + /// + [UnsupportedOSPlatform("browser")] + private void SpinUntilFastPathFinishes() + { + SpinWait spinner = default; + while (m_threadsTryingFastPathCount > 0) + { + spinner.SpinOnce(); + } + } + /// /// Blocks the current thread until it can enter the , /// using a 32-bit unsigned integer to measure the time interval, @@ -336,15 +390,9 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo return false; } - // Perf: If there is no wait handle, we can try entering the semaphore without using the lock. - // Otherwise, we would actually need to take the lock to avoid race conditions when calling m_waitHandle.Reset() - if (m_waitHandle is null) + if (TryWaitFastPath()) { - int currentCount = m_currentCount; - if (currentCount > 0 && Interlocked.CompareExchange(ref m_currentCount, currentCount - 1, currentCount) == currentCount) - { - return true; - } + return true; } long startTime = 0; @@ -385,6 +433,10 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo } } Monitor.Enter(m_lockObjAndDisposed, ref lockTaken); + if (m_threadsTryingFastPathCount > 0) + { + SpinUntilFastPathFinishes(); + } m_waitCount++; // If there are any async waiters, for fairness we'll get in line behind @@ -699,8 +751,17 @@ private Task WaitAsyncCore(long millisecondsTimeout, CancellationToken can if (cancellationToken.IsCancellationRequested) return Task.FromCanceled(cancellationToken); + if (TryWaitFastPath()) + { + return Task.FromResult(true); + } + lock (m_lockObjAndDisposed) { + if (m_threadsTryingFastPathCount > 0) + { + SpinUntilFastPathFinishes(); + } // If there are counts available, allow this waiter to succeed. if (m_currentCount > 0) { @@ -813,6 +874,10 @@ private async Task WaitUntilCountOrTimeoutAsync(TaskNode asyncWaiter, long // we no longer hold the lock. As such, acquire it. lock (m_lockObjAndDisposed) { + if (m_threadsTryingFastPathCount > 0) + { + SpinUntilFastPathFinishes(); + } // Remove the task from the list. If we're successful in doing so, // we know that no one else has tried to complete this waiter yet, // so we can safely cancel or timeout. @@ -839,6 +904,38 @@ public int Release() return Release(1); } + /// + /// Tries to release the semaphore without taking the lock. + /// + /// The previous count of the if successful; otherwise, -1. + private int TryReleaseFastPath(int releaseCount) + { + int result = -1; + if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) + { + Interlocked.Increment(ref m_threadsTryingFastPathCount); + // It's possible that the lock is taken by now, don't attempt the fast path in that case. + // However if it's not taken by now, it's safe to attempt the fast path. + // If the lock is taken after checking this condition, because m_threadsTryingFastPathCount is already greater than 0, + // the thread holding the lock wouldn't start its operations. + // The wait handle and async head need to be null and wait count to be zero to take the fast path. + // Otherwise we have to follow the slow path taking the lock to avoid race conditions. + // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, did its work and released it + // before this thread updated m_threadsTryingFastPathCount. + if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) + { + int currentCount = m_currentCount; + if (m_maxCount - currentCount >= releaseCount && + Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) + { + result = currentCount; + } + } + Interlocked.Decrement(ref m_threadsTryingFastPathCount); + } + return result; + } + /// /// Exits the a specified number of times. /// @@ -860,19 +957,20 @@ public int Release(int releaseCount) nameof(releaseCount), releaseCount, SR.SemaphoreSlim_Release_CountWrong); } - if (m_waitCount == 0 && m_waitHandle is null && m_asyncHead is null) + int fastPathResult = TryReleaseFastPath(releaseCount); + if (fastPathResult >= 0) { - int currentCount = m_currentCount; - if (m_maxCount - currentCount >= releaseCount && Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) - { - return currentCount; - } + return fastPathResult; } int returnCount; lock (m_lockObjAndDisposed) { + if (m_threadsTryingFastPathCount > 0) + { + SpinUntilFastPathFinishes(); + } // Read the m_currentCount into a local variable to avoid unnecessary volatile accesses inside the lock. int currentCount = m_currentCount; returnCount = currentCount; From 1b0dbb05d6b613065e7c8135428432933cfe9a31 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Thu, 21 Aug 2025 15:44:14 -0700 Subject: [PATCH 03/13] Support TryWaitFastPath/SpinUntilFastPathFinishes on browser too --- .../src/System/Threading/SemaphoreSlim.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index 13ac51af800e23..6fa5bfd5971f09 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -323,7 +323,6 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) /// Tries a fast path to wait on the semaphore without taking the lock. /// /// true if the fast path succeeded; otherwise, false. - [UnsupportedOSPlatform("browser")] private bool TryWaitFastPath() { bool result = false; @@ -354,7 +353,6 @@ private bool TryWaitFastPath() /// /// Blocks the current thread until all the threads trying the fast path finish. /// - [UnsupportedOSPlatform("browser")] private void SpinUntilFastPathFinishes() { SpinWait spinner = default; From f8ee2b0c45c0ed843cd9f3fa2cddb608d2c47214 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Thu, 21 Aug 2025 18:34:45 -0700 Subject: [PATCH 04/13] Fix race conditions --- .../src/System/Threading/SemaphoreSlim.cs | 34 +++++++++++++++---- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index 6fa5bfd5971f09..1f134f49afcb9b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -52,6 +52,11 @@ public class SemaphoreSlim : IDisposable // Boolean value indicates whether the instance has been disposed. private readonly StrongBox m_lockObjAndDisposed; + // Indicates whether some thread is executing within the lock region. + // In WaitUntilCountOrTimeoutAsync, the lock might be acquired even if the thread is holding it. + // Therefore, increment and decrement this counter as the locks are acquired and released. + private volatile int m_inLockRegion; + // Act as the semaphore wait handle, it's lazily initialized if needed, the first WaitHandle call initialize it // and wait an release sets and resets it respectively as long as it is not null private volatile ManualResetEvent? m_waitHandle; @@ -111,6 +116,7 @@ public WaitHandle AvailableWaitHandle // lock the count to avoid multiple threads initializing the handle if it is null lock (m_lockObjAndDisposed) { + Interlocked.Increment(ref m_inLockRegion); if (m_threadsTryingFastPathCount > 0) { SpinUntilFastPathFinishes(); @@ -118,6 +124,7 @@ public WaitHandle AvailableWaitHandle // The initial state for the wait handle is true if the count is greater than zero // false otherwise m_waitHandle ??= new ManualResetEvent(m_currentCount != 0); + Interlocked.Decrement(ref m_inLockRegion); } } @@ -326,7 +333,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) private bool TryWaitFastPath() { bool result = false; - if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null) + if (m_inLockRegion == 0 && m_waitHandle is null) { Interlocked.Increment(ref m_threadsTryingFastPathCount); // It's possible that the lock is taken by now, don't attempt the fast path in that case. @@ -334,9 +341,9 @@ private bool TryWaitFastPath() // If the lock is taken after checking this condition, because m_threadsTryingFastPathCount is already greater than 0, // the thread holding the lock wouldn't start its operations. // If the wait handle is not null, we have to follow the slow path taking the lock to avoid race conditions. - // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, did its work and released it + // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, done its work and released it // before this thread updated m_threadsTryingFastPathCount. - if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null) + if (m_inLockRegion == 0 && m_waitHandle is null) { int currentCount = m_currentCount; if (currentCount > 0 && @@ -431,6 +438,8 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo } } Monitor.Enter(m_lockObjAndDisposed, ref lockTaken); + Interlocked.Increment(ref m_inLockRegion); + if (m_threadsTryingFastPathCount > 0) { SpinUntilFastPathFinishes(); @@ -500,6 +509,7 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo { m_waitCount--; Monitor.Exit(m_lockObjAndDisposed); + Interlocked.Decrement(ref m_inLockRegion); } // Unregister the cancellation callback. @@ -756,6 +766,7 @@ private Task WaitAsyncCore(long millisecondsTimeout, CancellationToken can lock (m_lockObjAndDisposed) { + Interlocked.Increment(ref m_inLockRegion); if (m_threadsTryingFastPathCount > 0) { SpinUntilFastPathFinishes(); @@ -765,10 +776,12 @@ private Task WaitAsyncCore(long millisecondsTimeout, CancellationToken can { --m_currentCount; if (m_waitHandle is not null && m_currentCount == 0) m_waitHandle.Reset(); + Interlocked.Decrement(ref m_inLockRegion); return Task.FromResult(true); } else if (millisecondsTimeout == 0) { + Interlocked.Decrement(ref m_inLockRegion); // No counts, if timeout is zero fail fast return Task.FromResult(false); } @@ -779,9 +792,11 @@ private Task WaitAsyncCore(long millisecondsTimeout, CancellationToken can { Debug.Assert(m_currentCount == 0, "m_currentCount should never be negative"); TaskNode asyncWaiter = CreateAndAddAsyncWaiter(); - return (millisecondsTimeout == Timeout.Infinite && !cancellationToken.CanBeCanceled) ? + Task result = (millisecondsTimeout == Timeout.Infinite && !cancellationToken.CanBeCanceled) ? asyncWaiter : WaitUntilCountOrTimeoutAsync(asyncWaiter, millisecondsTimeout, cancellationToken); + Interlocked.Decrement(ref m_inLockRegion); + return result; } } } @@ -872,6 +887,7 @@ private async Task WaitUntilCountOrTimeoutAsync(TaskNode asyncWaiter, long // we no longer hold the lock. As such, acquire it. lock (m_lockObjAndDisposed) { + Interlocked.Increment(ref m_inLockRegion); if (m_threadsTryingFastPathCount > 0) { SpinUntilFastPathFinishes(); @@ -882,8 +898,10 @@ private async Task WaitUntilCountOrTimeoutAsync(TaskNode asyncWaiter, long if (RemoveAsyncWaiter(asyncWaiter)) { cancellationToken.ThrowIfCancellationRequested(); // cancellation occurred + Interlocked.Decrement(ref m_inLockRegion); return false; // timeout occurred } + Interlocked.Decrement(ref m_inLockRegion); } // The waiter had already been removed, which means it's already completed or is about to @@ -909,7 +927,7 @@ public int Release() private int TryReleaseFastPath(int releaseCount) { int result = -1; - if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) + if (m_inLockRegion == 0 && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) { Interlocked.Increment(ref m_threadsTryingFastPathCount); // It's possible that the lock is taken by now, don't attempt the fast path in that case. @@ -918,9 +936,9 @@ private int TryReleaseFastPath(int releaseCount) // the thread holding the lock wouldn't start its operations. // The wait handle and async head need to be null and wait count to be zero to take the fast path. // Otherwise we have to follow the slow path taking the lock to avoid race conditions. - // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, did its work and released it + // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, done its work and released it // before this thread updated m_threadsTryingFastPathCount. - if (!Monitor.IsEntered(m_lockObjAndDisposed) && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) + if (m_inLockRegion == 0 && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) { int currentCount = m_currentCount; if (m_maxCount - currentCount >= releaseCount && @@ -965,6 +983,7 @@ public int Release(int releaseCount) lock (m_lockObjAndDisposed) { + Interlocked.Increment(ref m_inLockRegion); if (m_threadsTryingFastPathCount > 0) { SpinUntilFastPathFinishes(); @@ -1033,6 +1052,7 @@ public int Release(int releaseCount) { m_waitHandle.Set(); } + Interlocked.Decrement(ref m_inLockRegion); } // And return the count From 83ca4ad42472fa2d3b1d1b3eb19c5907cb3f8531 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Fri, 19 Sep 2025 11:13:00 -0700 Subject: [PATCH 05/13] Inline fast path + Validate argument in Wait(int) --- .../src/System/Threading/SemaphoreSlim.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index 1f134f49afcb9b..62457c6336596d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -295,6 +295,13 @@ public bool Wait(int millisecondsTimeout) #if TARGET_WASI if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185 #endif + + if (millisecondsTimeout < -1) + { + throw new ArgumentOutOfRangeException( + nameof(millisecondsTimeout), millisecondsTimeout, SR.SemaphoreSlim_Wait_TimeoutWrong); + } + return WaitCore(millisecondsTimeout, CancellationToken.None); } @@ -330,6 +337,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) /// Tries a fast path to wait on the semaphore without taking the lock. /// /// true if the fast path succeeded; otherwise, false. + [MethodImpl(MethodImplOptions.AggressiveInlining)] private bool TryWaitFastPath() { bool result = false; @@ -924,6 +932,7 @@ public int Release() /// Tries to release the semaphore without taking the lock. /// /// The previous count of the if successful; otherwise, -1. + [MethodImpl(MethodImplOptions.AggressiveInlining)] private int TryReleaseFastPath(int releaseCount) { int result = -1; From a6dcfd58fbb1af4e62647b38de7340e020dbcd7d Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Fri, 19 Sep 2025 11:16:56 -0700 Subject: [PATCH 06/13] Measure fast path usage --- .../src/System/Threading/SemaphoreSlim.cs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index 62457c6336596d..ed4b7121c7d7e0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -75,6 +75,12 @@ public class SemaphoreSlim : IDisposable // all threads finish their attempt to use the fast path. private volatile int m_threadsTryingFastPathCount; + private int m_fastPathWaitAcquires; + private int m_slowPathWaitAcquires; + + private int m_fastPathReleaseAcquires; + private int m_slowPathReleaseAcquires; + // Task in a linked list of asynchronous waiters private sealed class TaskNode : Task { @@ -296,12 +302,22 @@ public bool Wait(int millisecondsTimeout) if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185 #endif + if (millisecondsTimeout == -2) + { + Internal.Console.WriteLine("Fast path acquires " + m_fastPathWaitAcquires); + Internal.Console.WriteLine("Slow path acquires " + m_slowPathWaitAcquires); + Internal.Console.WriteLine("Fast path releases " + m_fastPathReleaseAcquires); + Internal.Console.WriteLine("Slow path releases " + m_slowPathReleaseAcquires); + return false; + } + if (millisecondsTimeout < -1) { throw new ArgumentOutOfRangeException( nameof(millisecondsTimeout), millisecondsTimeout, SR.SemaphoreSlim_Wait_TimeoutWrong); } + return WaitCore(millisecondsTimeout, CancellationToken.None); } @@ -324,6 +340,15 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185 #endif + if (millisecondsTimeout == -2) + { + Internal.Console.WriteLine("Fast path acquires " + m_fastPathWaitAcquires); + Internal.Console.WriteLine("Slow path acquires " + m_slowPathWaitAcquires); + Internal.Console.WriteLine("Fast path releases " + m_fastPathReleaseAcquires); + Internal.Console.WriteLine("Slow path releases " + m_slowPathReleaseAcquires); + return false; + } + if (millisecondsTimeout < -1) { throw new ArgumentOutOfRangeException( @@ -357,6 +382,7 @@ private bool TryWaitFastPath() if (currentCount > 0 && Interlocked.CompareExchange(ref m_currentCount, currentCount - 1, currentCount) == currentCount) { + Interlocked.Increment(ref m_fastPathWaitAcquires); result = true; } } @@ -408,6 +434,7 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo return true; } + Interlocked.Increment(ref m_slowPathWaitAcquires); long startTime = 0; if (millisecondsTimeout != Timeout.Infinite && millisecondsTimeout > 0) { @@ -954,6 +981,7 @@ private int TryReleaseFastPath(int releaseCount) Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) { result = currentCount; + Interlocked.Increment(ref m_fastPathReleaseAcquires); } } Interlocked.Decrement(ref m_threadsTryingFastPathCount); @@ -988,6 +1016,8 @@ public int Release(int releaseCount) return fastPathResult; } + Interlocked.Increment(ref m_slowPathReleaseAcquires); + int returnCount; lock (m_lockObjAndDisposed) From 531fac66aa107d25ec5e29cfa6760da8efc24b54 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Tue, 23 Sep 2025 16:57:49 -0700 Subject: [PATCH 07/13] Enable fast paths with only one interlocked operation --- .../src/System/Threading/SemaphoreSlim.cs | 239 ++++++------------ 1 file changed, 72 insertions(+), 167 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index ed4b7121c7d7e0..fce0ca7f2f8ca8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -52,11 +52,6 @@ public class SemaphoreSlim : IDisposable // Boolean value indicates whether the instance has been disposed. private readonly StrongBox m_lockObjAndDisposed; - // Indicates whether some thread is executing within the lock region. - // In WaitUntilCountOrTimeoutAsync, the lock might be acquired even if the thread is holding it. - // Therefore, increment and decrement this counter as the locks are acquired and released. - private volatile int m_inLockRegion; - // Act as the semaphore wait handle, it's lazily initialized if needed, the first WaitHandle call initialize it // and wait an release sets and resets it respectively as long as it is not null private volatile ManualResetEvent? m_waitHandle; @@ -70,15 +65,7 @@ public class SemaphoreSlim : IDisposable // No maximum constant private const int NO_MAXIMUM = int.MaxValue; - // Used to track if we are attempting the fast path (without taking the lock). - // Therefore, if another thread takes the lock, shouldn't start its operations until - // all threads finish their attempt to use the fast path. - private volatile int m_threadsTryingFastPathCount; - - private int m_fastPathWaitAcquires; private int m_slowPathWaitAcquires; - - private int m_fastPathReleaseAcquires; private int m_slowPathReleaseAcquires; // Task in a linked list of asynchronous waiters @@ -95,7 +82,7 @@ internal TaskNode() : base((object?)null, TaskCreationOptions.RunContinuationsAs /// Gets the current count of the . /// /// The current count of the . - public int CurrentCount => m_currentCount; + public int CurrentCount => Math.Max(m_currentCount, 0); /// /// Returns a that can be used to wait on the semaphore. @@ -122,15 +109,9 @@ public WaitHandle AvailableWaitHandle // lock the count to avoid multiple threads initializing the handle if it is null lock (m_lockObjAndDisposed) { - Interlocked.Increment(ref m_inLockRegion); - if (m_threadsTryingFastPathCount > 0) - { - SpinUntilFastPathFinishes(); - } // The initial state for the wait handle is true if the count is greater than zero // false otherwise m_waitHandle ??= new ManualResetEvent(m_currentCount != 0); - Interlocked.Decrement(ref m_inLockRegion); } } @@ -304,9 +285,7 @@ public bool Wait(int millisecondsTimeout) if (millisecondsTimeout == -2) { - Internal.Console.WriteLine("Fast path acquires " + m_fastPathWaitAcquires); Internal.Console.WriteLine("Slow path acquires " + m_slowPathWaitAcquires); - Internal.Console.WriteLine("Fast path releases " + m_fastPathReleaseAcquires); Internal.Console.WriteLine("Slow path releases " + m_slowPathReleaseAcquires); return false; } @@ -342,9 +321,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) if (millisecondsTimeout == -2) { - Internal.Console.WriteLine("Fast path acquires " + m_fastPathWaitAcquires); Internal.Console.WriteLine("Slow path acquires " + m_slowPathWaitAcquires); - Internal.Console.WriteLine("Fast path releases " + m_fastPathReleaseAcquires); Internal.Console.WriteLine("Slow path releases " + m_slowPathReleaseAcquires); return false; } @@ -359,48 +336,26 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) } /// - /// Tries a fast path to wait on the semaphore without taking the lock. + /// Attempts to enter the immediately, and returns a value that indicates whether the + /// attempt was successful. /// - /// true if the fast path succeeded; otherwise, false. + /// true if the current thread successfully entered the ; otherwise, false. [MethodImpl(MethodImplOptions.AggressiveInlining)] - private bool TryWaitFastPath() + private bool TryAcquireSemaphore() { - bool result = false; - if (m_inLockRegion == 0 && m_waitHandle is null) + if (m_currentCount <= 0) { - Interlocked.Increment(ref m_threadsTryingFastPathCount); - // It's possible that the lock is taken by now, don't attempt the fast path in that case. - // However if it's not taken by now, it's safe to attempt the fast path. - // If the lock is taken after checking this condition, because m_threadsTryingFastPathCount is already greater than 0, - // the thread holding the lock wouldn't start its operations. - // If the wait handle is not null, we have to follow the slow path taking the lock to avoid race conditions. - // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, done its work and released it - // before this thread updated m_threadsTryingFastPathCount. - if (m_inLockRegion == 0 && m_waitHandle is null) - { - int currentCount = m_currentCount; - if (currentCount > 0 && - Interlocked.CompareExchange(ref m_currentCount, currentCount - 1, currentCount) == currentCount) - { - Interlocked.Increment(ref m_fastPathWaitAcquires); - result = true; - } - } - Interlocked.Decrement(ref m_threadsTryingFastPathCount); + return false; } - return result; - } - /// - /// Blocks the current thread until all the threads trying the fast path finish. - /// - private void SpinUntilFastPathFinishes() - { - SpinWait spinner = default; - while (m_threadsTryingFastPathCount > 0) + int newCount = Interlocked.Decrement(ref m_currentCount); + if (newCount < 0) { - spinner.SpinOnce(); + Interlocked.Increment(ref m_currentCount); + return false; } + + return true; } /// @@ -429,9 +384,15 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo return false; } - if (TryWaitFastPath()) + // Wait Fast Path: If the count is greater than zero, try to acquire the semaphore. + // Perf: Check if it's possible to decrease the current count with an Interlocked operation instead of taking the Monitor lock. + // Only attempt this if there is no wait handle, otherwise it's not possible to guarantee that the wait handle is in the correct state with this optimization. + if (m_waitHandle is null) { - return true; + if (TryAcquireSemaphore()) + { + return true; + } } Interlocked.Increment(ref m_slowPathWaitAcquires); @@ -473,12 +434,7 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo } } Monitor.Enter(m_lockObjAndDisposed, ref lockTaken); - Interlocked.Increment(ref m_inLockRegion); - if (m_threadsTryingFastPathCount > 0) - { - SpinUntilFastPathFinishes(); - } m_waitCount++; // If there are any async waiters, for fairness we'll get in line behind @@ -492,43 +448,15 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo // There are no async waiters, so we can proceed with normal synchronous waiting. else { - // If the count > 0 we are good to move on. - // If not, then wait if we were given allowed some wait duration - - OperationCanceledException? oce = null; - - if (m_currentCount == 0) + // If we cannot wait, then try to acquire the semaphore. + // If the new count becomes negative, that means that the count was zero + // so we should revert this invalid operation and return false. + if (millisecondsTimeout == 0) { - if (millisecondsTimeout == 0) - { - return false; - } - - // Prepare for the main wait... - // wait until the count become greater than zero or the timeout is expired - try - { - waitSuccessful = WaitUntilCountOrTimeout(millisecondsTimeout, startTime, cancellationToken); - } - catch (OperationCanceledException e) { oce = e; } - } - - // Now try to acquire. We prioritize acquisition over cancellation/timeout so that we don't - // lose any counts when there are asynchronous waiters in the mix. Asynchronous waiters - // defer to synchronous waiters in priority, which means that if it's possible an asynchronous - // waiter didn't get released because a synchronous waiter was present, we need to ensure - // that synchronous waiter succeeds so that they have a chance to release. - Debug.Assert(!waitSuccessful || m_currentCount > 0, - "If the wait was successful, there should be count available."); - if (m_currentCount > 0) - { - waitSuccessful = true; - m_currentCount--; - } - else if (oce is not null) - { - throw oce; + return TryAcquireSemaphore(); } + + waitSuccessful = WaitUntilCountOrTimeout(millisecondsTimeout, startTime, cancellationToken); // Exposing wait handle which is lazily initialized if needed if (m_waitHandle is not null && m_currentCount == 0) @@ -544,7 +472,6 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo { m_waitCount--; Monitor.Exit(m_lockObjAndDisposed); - Interlocked.Decrement(ref m_inLockRegion); } // Unregister the cancellation callback. @@ -576,9 +503,14 @@ private bool WaitUntilCountOrTimeout(long millisecondsTimeout, long startTime, C #endif int monitorWaitMilliseconds = Timeout.Infinite; - // Wait on the monitor as long as the count is zero - while (m_currentCount == 0) + // Wait on the monitor as long we cannot acquire the semaphore + while (true) { + if (TryAcquireSemaphore()) + { + return true; + } + // If cancelled, we throw. Trying to wait could lead to deadlock. cancellationToken.ThrowIfCancellationRequested(); @@ -625,8 +557,6 @@ private bool WaitUntilCountOrTimeout(long millisecondsTimeout, long startTime, C return false; } } - - return true; } /// @@ -794,29 +724,26 @@ private Task WaitAsyncCore(long millisecondsTimeout, CancellationToken can if (cancellationToken.IsCancellationRequested) return Task.FromCanceled(cancellationToken); - if (TryWaitFastPath()) + // Perf: Check if it's possible to decrease the current count with an Interlocked operation instead of taking the Monitor lock. + // Only attempt this if there is no wait handle, otherwise it's not possible to guarantee that the wait handle is in the correct state with this optimization. + if (m_waitHandle is null) { - return Task.FromResult(true); + if (TryAcquireSemaphore()) + { + return Task.FromResult(true); + } } lock (m_lockObjAndDisposed) { - Interlocked.Increment(ref m_inLockRegion); - if (m_threadsTryingFastPathCount > 0) - { - SpinUntilFastPathFinishes(); - } // If there are counts available, allow this waiter to succeed. - if (m_currentCount > 0) + if (TryAcquireSemaphore()) { - --m_currentCount; if (m_waitHandle is not null && m_currentCount == 0) m_waitHandle.Reset(); - Interlocked.Decrement(ref m_inLockRegion); return Task.FromResult(true); } else if (millisecondsTimeout == 0) { - Interlocked.Decrement(ref m_inLockRegion); // No counts, if timeout is zero fail fast return Task.FromResult(false); } @@ -830,7 +757,6 @@ private Task WaitAsyncCore(long millisecondsTimeout, CancellationToken can Task result = (millisecondsTimeout == Timeout.Infinite && !cancellationToken.CanBeCanceled) ? asyncWaiter : WaitUntilCountOrTimeoutAsync(asyncWaiter, millisecondsTimeout, cancellationToken); - Interlocked.Decrement(ref m_inLockRegion); return result; } } @@ -922,21 +848,14 @@ private async Task WaitUntilCountOrTimeoutAsync(TaskNode asyncWaiter, long // we no longer hold the lock. As such, acquire it. lock (m_lockObjAndDisposed) { - Interlocked.Increment(ref m_inLockRegion); - if (m_threadsTryingFastPathCount > 0) - { - SpinUntilFastPathFinishes(); - } // Remove the task from the list. If we're successful in doing so, // we know that no one else has tried to complete this waiter yet, // so we can safely cancel or timeout. if (RemoveAsyncWaiter(asyncWaiter)) { cancellationToken.ThrowIfCancellationRequested(); // cancellation occurred - Interlocked.Decrement(ref m_inLockRegion); return false; // timeout occurred } - Interlocked.Decrement(ref m_inLockRegion); } // The waiter had already been removed, which means it's already completed or is about to @@ -955,40 +874,6 @@ public int Release() return Release(1); } - /// - /// Tries to release the semaphore without taking the lock. - /// - /// The previous count of the if successful; otherwise, -1. - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private int TryReleaseFastPath(int releaseCount) - { - int result = -1; - if (m_inLockRegion == 0 && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) - { - Interlocked.Increment(ref m_threadsTryingFastPathCount); - // It's possible that the lock is taken by now, don't attempt the fast path in that case. - // However if it's not taken by now, it's safe to attempt the fast path. - // If the lock is taken after checking this condition, because m_threadsTryingFastPathCount is already greater than 0, - // the thread holding the lock wouldn't start its operations. - // The wait handle and async head need to be null and wait count to be zero to take the fast path. - // Otherwise we have to follow the slow path taking the lock to avoid race conditions. - // We need to re-evaluate the conditions before proceeding as another thread might have taken the lock, done its work and released it - // before this thread updated m_threadsTryingFastPathCount. - if (m_inLockRegion == 0 && m_waitHandle is null && m_waitCount == 0 && m_asyncHead is null) - { - int currentCount = m_currentCount; - if (m_maxCount - currentCount >= releaseCount && - Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) - { - result = currentCount; - Interlocked.Increment(ref m_fastPathReleaseAcquires); - } - } - Interlocked.Decrement(ref m_threadsTryingFastPathCount); - } - return result; - } - /// /// Exits the a specified number of times. /// @@ -1010,23 +895,44 @@ public int Release(int releaseCount) nameof(releaseCount), releaseCount, SR.SemaphoreSlim_Release_CountWrong); } - int fastPathResult = TryReleaseFastPath(releaseCount); - if (fastPathResult >= 0) + if (m_currentCount > 0) { - return fastPathResult; + int previousCount = Interlocked.Add(ref m_currentCount, releaseCount) - releaseCount; + if (previousCount + releaseCount > m_maxCount) + { + // Revert the increment + Interlocked.Add(ref m_currentCount, -releaseCount); + throw new SemaphoreFullException(); + } + else if (previousCount <= 0) + { + // Revert the increment + Interlocked.Add(ref m_currentCount, -releaseCount); + } + else + { + return previousCount; + } } Interlocked.Increment(ref m_slowPathReleaseAcquires); int returnCount; + // If m_currentCount was not greater than 0, it may be negative if some threads attempted to acquire + // the semaphore during the wait fast path but failed. + // In this case, the threads themselves will make the count back to zero after a little while, spin until then. + SpinWait spinner = default; + while (m_currentCount < 0) + { + spinner.SpinOnce(); + } + + // Once it became zero, we can take the lock knowing that no threads executing a wait operation will + // be able to modify the count until we release the lock. + lock (m_lockObjAndDisposed) { - Interlocked.Increment(ref m_inLockRegion); - if (m_threadsTryingFastPathCount > 0) - { - SpinUntilFastPathFinishes(); - } // Read the m_currentCount into a local variable to avoid unnecessary volatile accesses inside the lock. int currentCount = m_currentCount; returnCount = currentCount; @@ -1091,7 +997,6 @@ public int Release(int releaseCount) { m_waitHandle.Set(); } - Interlocked.Decrement(ref m_inLockRegion); } // And return the count From 2022f497dae91e6db611ddfde83007795d321f0d Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Tue, 23 Sep 2025 21:06:29 -0700 Subject: [PATCH 08/13] Nit --- .../src/System/Threading/SemaphoreSlim.cs | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index fce0ca7f2f8ca8..7a1b32e1bd47c0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -40,7 +40,7 @@ public class SemaphoreSlim : IDisposable // The number of synchronously waiting threads, it is set to zero in the constructor and increments before blocking the // threading and decrements it back after that. It is used as flag for the release call to know if there are // waiting threads in the monitor or not. - private volatile int m_waitCount; + private int m_waitCount; /// /// This is used to help prevent waking more waiters than necessary. It's not perfect and sometimes more waiters than @@ -57,7 +57,7 @@ public class SemaphoreSlim : IDisposable private volatile ManualResetEvent? m_waitHandle; // Head of list representing asynchronous waits on the semaphore. - private volatile TaskNode? m_asyncHead; + private TaskNode? m_asyncHead; // Tail of list representing asynchronous waits on the semaphore. private TaskNode? m_asyncTail; @@ -384,8 +384,8 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo return false; } - // Wait Fast Path: If the count is greater than zero, try to acquire the semaphore. - // Perf: Check if it's possible to decrease the current count with an Interlocked operation instead of taking the Monitor lock. + // Fast Path: If the count is greater than zero, try to acquire the semaphore. + // Check if it's possible to decrease the current count with an Interlocked operation instead of taking the Monitor lock. // Only attempt this if there is no wait handle, otherwise it's not possible to guarantee that the wait handle is in the correct state with this optimization. if (m_waitHandle is null) { @@ -449,13 +449,13 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo else { // If we cannot wait, then try to acquire the semaphore. - // If the new count becomes negative, that means that the count was zero + // If the new count becomes negative, that means that the count was zero // so we should revert this invalid operation and return false. if (millisecondsTimeout == 0) { return TryAcquireSemaphore(); } - + waitSuccessful = WaitUntilCountOrTimeout(millisecondsTimeout, startTime, cancellationToken); // Exposing wait handle which is lazily initialized if needed @@ -895,13 +895,14 @@ public int Release(int releaseCount) nameof(releaseCount), releaseCount, SR.SemaphoreSlim_Release_CountWrong); } + // Fast path: If the count is greater than zero, try to release the semaphore without taking the lock. + // If it's zero or less, we need to take the lock to ensure that we properly wake up waiters + // and potentially update m_waitHandle. if (m_currentCount > 0) { int previousCount = Interlocked.Add(ref m_currentCount, releaseCount) - releaseCount; - if (previousCount + releaseCount > m_maxCount) + if (previousCount > m_maxCount - releaseCount) { - // Revert the increment - Interlocked.Add(ref m_currentCount, -releaseCount); throw new SemaphoreFullException(); } else if (previousCount <= 0) @@ -919,7 +920,7 @@ public int Release(int releaseCount) int returnCount; - // If m_currentCount was not greater than 0, it may be negative if some threads attempted to acquire + // If m_currentCount was not greater than 0, it may be negative if some threads attempted to acquire // the semaphore during the wait fast path but failed. // In this case, the threads themselves will make the count back to zero after a little while, spin until then. SpinWait spinner = default; @@ -928,8 +929,9 @@ public int Release(int releaseCount) spinner.SpinOnce(); } - // Once it became zero, we can take the lock knowing that no threads executing a wait operation will - // be able to modify the count until we release the lock. + // The current count must be 0 and we can take the lock knowing that no threads executing a wait operation + // will be able to modify the count until we release the lock. + Debug.Assert(m_currentCount == 0, "m_currentCount should never be negative here"); lock (m_lockObjAndDisposed) { From 9cebb957a643b2cca8f3127b93d11569df9268d5 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Wed, 24 Sep 2025 12:38:51 -0700 Subject: [PATCH 09/13] Remove slow path variables --- .../src/System/Threading/SemaphoreSlim.cs | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index 7a1b32e1bd47c0..f5236688576498 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -65,9 +65,6 @@ public class SemaphoreSlim : IDisposable // No maximum constant private const int NO_MAXIMUM = int.MaxValue; - private int m_slowPathWaitAcquires; - private int m_slowPathReleaseAcquires; - // Task in a linked list of asynchronous waiters private sealed class TaskNode : Task { @@ -283,13 +280,6 @@ public bool Wait(int millisecondsTimeout) if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185 #endif - if (millisecondsTimeout == -2) - { - Internal.Console.WriteLine("Slow path acquires " + m_slowPathWaitAcquires); - Internal.Console.WriteLine("Slow path releases " + m_slowPathReleaseAcquires); - return false; - } - if (millisecondsTimeout < -1) { throw new ArgumentOutOfRangeException( @@ -319,13 +309,6 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185 #endif - if (millisecondsTimeout == -2) - { - Internal.Console.WriteLine("Slow path acquires " + m_slowPathWaitAcquires); - Internal.Console.WriteLine("Slow path releases " + m_slowPathReleaseAcquires); - return false; - } - if (millisecondsTimeout < -1) { throw new ArgumentOutOfRangeException( @@ -395,7 +378,6 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo } } - Interlocked.Increment(ref m_slowPathWaitAcquires); long startTime = 0; if (millisecondsTimeout != Timeout.Infinite && millisecondsTimeout > 0) { @@ -916,8 +898,6 @@ public int Release(int releaseCount) } } - Interlocked.Increment(ref m_slowPathReleaseAcquires); - int returnCount; // If m_currentCount was not greater than 0, it may be negative if some threads attempted to acquire From 3eae7108068c19dc24ba6239138cc76844ddb71f Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Wed, 24 Sep 2025 13:40:45 -0700 Subject: [PATCH 10/13] Fix race condition --- .../src/System/Threading/SemaphoreSlim.cs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index f5236688576498..e810351deccef1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -880,22 +880,14 @@ public int Release(int releaseCount) // Fast path: If the count is greater than zero, try to release the semaphore without taking the lock. // If it's zero or less, we need to take the lock to ensure that we properly wake up waiters // and potentially update m_waitHandle. - if (m_currentCount > 0) + int currentCount = m_currentCount; + if (currentCount > 0 && Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) { - int previousCount = Interlocked.Add(ref m_currentCount, releaseCount) - releaseCount; - if (previousCount > m_maxCount - releaseCount) + if (m_maxCount - currentCount < releaseCount) { throw new SemaphoreFullException(); } - else if (previousCount <= 0) - { - // Revert the increment - Interlocked.Add(ref m_currentCount, -releaseCount); - } - else - { - return previousCount; - } + return currentCount; } int returnCount; @@ -913,6 +905,7 @@ public int Release(int releaseCount) // will be able to modify the count until we release the lock. Debug.Assert(m_currentCount == 0, "m_currentCount should never be negative here"); + lock (m_lockObjAndDisposed) { // Read the m_currentCount into a local variable to avoid unnecessary volatile accesses inside the lock. From 2bd55b3a835cc729e4f5098817723493d9ed167a Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Wed, 24 Sep 2025 14:27:51 -0700 Subject: [PATCH 11/13] Fix build --- .../src/System/Threading/SemaphoreSlim.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index e810351deccef1..09c11601a0d9a9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -881,12 +881,9 @@ public int Release(int releaseCount) // If it's zero or less, we need to take the lock to ensure that we properly wake up waiters // and potentially update m_waitHandle. int currentCount = m_currentCount; - if (currentCount > 0 && Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) + if (currentCount > 0 && currentCount + releaseCount <= m_maxCount && + Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) { - if (m_maxCount - currentCount < releaseCount) - { - throw new SemaphoreFullException(); - } return currentCount; } @@ -909,7 +906,7 @@ public int Release(int releaseCount) lock (m_lockObjAndDisposed) { // Read the m_currentCount into a local variable to avoid unnecessary volatile accesses inside the lock. - int currentCount = m_currentCount; + currentCount = m_currentCount; returnCount = currentCount; // If the release count would result exceeding the maximum count, throw SemaphoreFullException. From 593a5d49269bb0021a3c55a9bb7bd1f3b002ae38 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Wed, 24 Sep 2025 20:38:23 -0700 Subject: [PATCH 12/13] Attempt to release while count is positive --- .../src/System/Threading/SemaphoreSlim.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index 09c11601a0d9a9..e270ed74dc51d4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -880,11 +880,14 @@ public int Release(int releaseCount) // Fast path: If the count is greater than zero, try to release the semaphore without taking the lock. // If it's zero or less, we need to take the lock to ensure that we properly wake up waiters // and potentially update m_waitHandle. - int currentCount = m_currentCount; - if (currentCount > 0 && currentCount + releaseCount <= m_maxCount && - Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) + while (m_currentCount > 0) { - return currentCount; + int currentCount = m_currentCount; + if (currentCount > 0 && currentCount + releaseCount <= m_maxCount && + Interlocked.CompareExchange(ref m_currentCount, currentCount + releaseCount, currentCount) == currentCount) + { + return currentCount; + } } int returnCount; @@ -900,8 +903,7 @@ public int Release(int releaseCount) // The current count must be 0 and we can take the lock knowing that no threads executing a wait operation // will be able to modify the count until we release the lock. - Debug.Assert(m_currentCount == 0, "m_currentCount should never be negative here"); - + Debug.Assert(m_currentCount == 0, "m_currentCount should be exactly zero before taking the lock"); lock (m_lockObjAndDisposed) { From 8a73cc61b75850578d4cf7bcee609ed5f8d7f2e8 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Wed, 24 Sep 2025 21:06:33 -0700 Subject: [PATCH 13/13] Fix build --- .../src/System/Threading/SemaphoreSlim.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index e270ed74dc51d4..82fb251e09e3be 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -908,7 +908,7 @@ public int Release(int releaseCount) lock (m_lockObjAndDisposed) { // Read the m_currentCount into a local variable to avoid unnecessary volatile accesses inside the lock. - currentCount = m_currentCount; + int currentCount = m_currentCount; returnCount = currentCount; // If the release count would result exceeding the maximum count, throw SemaphoreFullException.