Move to normal monitor lock due to expensive contention

Author: sakno

src/DotNext.Threading/Threading/Tasks/ManualResetCompletionSource.Lock.cs

@@ -5,37 +5,8 @@ namespace DotNext.Threading.Tasks;
 
 public partial class ManualResetCompletionSource
 {
-    private int lockState;
-
-    // a chance of lock contention for this instance is very low
-    // so monitor lock is too heavyweight for synchronization purposes
-    private protected void EnterLock()
-    {
-        ref var lockState = ref this.lockState;
-        if (Interlocked.Exchange(ref lockState, 1) is 1)
-            Contention(ref lockState);
-
-        [MethodImpl(MethodImplOptions.NoInlining)]
-        static void Contention(ref int lockState)
-        {
-            var spinner = new SpinWait();
-            do
-            {
-                spinner.SpinOnce();
-            }
-            while (Interlocked.Exchange(ref lockState, 1) is 1);
-        }
-    }
-
-    private bool TryEnterLock() => Interlocked.Exchange(ref lockState, 1) is 0;
-
-    private protected void ExitLock()
-    {
-        AssertLocked();
-
-        Volatile.Write(ref lockState, 0);
-    }
+    private protected object SyncRoot => cancellationCallback;
 
     [Conditional("DEBUG")]
-    private protected void AssertLocked() => Debug.Assert(lockState is not 0);
+    private protected void AssertLocked() => Debug.Assert(Monitor.IsEntered(SyncRoot));
 }

src/DotNext.Threading/Threading/Tasks/ManualResetCompletionSource.cs

@@ -47,8 +47,7 @@ private void CancellationRequested(object? expectedVersion, CancellationToken to
         if (versionAndStatus.Status is not ManualResetCompletionSourceStatus.Activated)
             goto exit;
 
-        EnterLock();
-        try
+        lock (SyncRoot)
         {
             if (versionAndStatus.Status is not ManualResetCompletionSourceStatus.Activated
                 || versionAndStatus.Version != (short)expectedVersion
@@ -58,14 +57,10 @@ private void CancellationRequested(object? expectedVersion, CancellationToken to
             // ensure that timeout or cancellation handler sets the status correctly
             Debug.Assert(versionAndStatus.Status is ManualResetCompletionSourceStatus.WaitForConsumption);
         }
-        finally
-        {
-            ExitLock();
-        }
 
         Resume();
 
-    exit:
+        exit:
         return;
     }
 
@@ -100,9 +95,9 @@ private CancellationState ResetCore(out short token)
     /// <returns>The version of the uncompleted task.</returns>
     public short Reset()
     {
-        EnterLock();
+        Monitor.Enter(SyncRoot);
         var stateCopy = ResetCore(out var token);
-        ExitLock();
+        Monitor.Exit(SyncRoot);
 
         stateCopy.Dispose();
         CleanUp();
@@ -119,10 +114,10 @@ public bool TryReset(out short token)
     {
         bool result;
 
-        if (result = TryEnterLock())
+        if (result = Monitor.TryEnter(SyncRoot))
         {
             var stateCopy = ResetCore(out token);
-            ExitLock();
+            Monitor.Exit(SyncRoot);
 
             stateCopy.Dispose();
             CleanUp();
@@ -190,26 +185,26 @@ private void OnCompleted(in Continuation continuation, short token)
 
         // code block doesn't have any calls leading to exceptions
         // so replace try-finally with manually cloned code
-        EnterLock();
+        Monitor.Enter(SyncRoot);
         if (token != versionAndStatus.Version)
         {
             errorMessage = ExceptionMessages.InvalidSourceToken;
-            ExitLock();
+            Monitor.Exit(SyncRoot);
             goto invalid_state;
         }
 
         switch (versionAndStatus.Status)
         {
             default:
                 errorMessage = ExceptionMessages.InvalidSourceState;
-                ExitLock();
+                Monitor.Exit(SyncRoot);
                 goto invalid_state;
             case ManualResetCompletionSourceStatus.WaitForConsumption:
-                ExitLock();
+                Monitor.Exit(SyncRoot);
                 break;
             case ManualResetCompletionSourceStatus.Activated:
                 this.continuation = continuation;
-                ExitLock();
+                Monitor.Exit(SyncRoot);
                 goto exit;
         }
 
@@ -294,27 +289,21 @@ public void OnCompleted(Action<object?> continuation, object? state, short token
     {
         Timeout.Validate(timeout);
 
-        // The source can be completed before the activation. Moreover, someone could try
-        // to complete the task during the activation concurrently. To avoid lock contention,
-        // use TryEnterLock(). If we can't acquire the lock, there is concurrent completion.
-        // In that case, do not activate the source and skip fast.
-        return TryEnterLock() ? ActivateSlow(timeout, token) : versionAndStatus.Version;
-    }
-
-    private short? ActivateSlow(TimeSpan timeout, CancellationToken token)
-    {
         short? result;
-        try
+        lock (SyncRoot)
         {
             switch (versionAndStatus.Status)
             {
-                case ManualResetCompletionSourceStatus.WaitForActivation when timeout == default:
-                    CompleteAsTimedOut();
-                    goto case ManualResetCompletionSourceStatus.WaitForConsumption;
                 case ManualResetCompletionSourceStatus.WaitForActivation:
-                    if (!state.Initialize(ref versionAndStatus, cancellationCallback, timeout, token))
+                    if (timeout == TimeSpan.Zero)
+                    {
+                        CompleteAsTimedOut();
+                    }
+                    else if (!state.Initialize(ref versionAndStatus, cancellationCallback, timeout, token))
+                    {
                         CompleteAsCanceled(token);
-                    
+                    }
+
                     goto case ManualResetCompletionSourceStatus.WaitForConsumption;
                 case ManualResetCompletionSourceStatus.WaitForConsumption:
                     result = versionAndStatus.Version;
@@ -324,10 +313,6 @@ public void OnCompleted(Action<object?> continuation, object? state, short token
                     break;
             }
         }
-        finally
-        {
-            ExitLock();
-        }
 
         return result;
     }

src/DotNext.Threading/Threading/Tasks/ValueTaskCompletionSource.T.cs

@@ -134,21 +134,16 @@ private unsafe bool SetResult<TArg>(object? completionData, short? completionTok
         Debug.Assert(func != null);
 
         bool result;
-        EnterLock();
-        try
+        lock (SyncRoot)
         {
             result = versionAndStatus.CanBeCompleted(completionToken);
             if (!result || !SetResult(func(arg), completionData))
                 goto exit;
         }
-        finally
-        {
-            ExitLock();
-        }
 
         Resume();
 
-    exit:
+        exit:
         return result;
     }
 
@@ -163,15 +158,10 @@ private bool SetResult(in Result<T> result, object? completionData = null)
     internal bool TrySetResult(object? completionData, short? completionToken, in Result<T> result, out bool resumable)
     {
         bool successful;
-        EnterLock();
-        try
+        lock (SyncRoot)
         {
             resumable = (successful = versionAndStatus.CanBeCompleted(completionToken)) && SetResult(in result, completionData);
         }
-        finally
-        {
-            ExitLock();
-        }
 
         return successful;
     }

src/DotNext.Threading/Threading/Tasks/ValueTaskCompletionSource.cs

@@ -68,18 +68,13 @@ private bool SetResult<TFactory>(object? completionData, short? completionToken,
         where TFactory : ISupplier<Exception?>
     {
         bool result;
-        EnterLock();
-        try
+        lock (SyncRoot)
         {
             result = versionAndStatus.CanBeCompleted(completionToken);
 
             if (!result || !SetResult(factory.Invoke(), completionData))
                 goto exit;
         }
-        finally
-        {
-            ExitLock();
-        }
 
         Resume();