Avoid unnecessary interlocked in ManualResetValueTaskSourceCore (dotnet/coreclr#20742)

When a ManualResetValueTaskSourceCore is used as the implementation for a ValueTask, it's set up for the operation, and then two things happen: a callback is hooked up at some point, and the operation completes at some point.  The former generally occurs before the latter, however there is a race condition, and so both paths currently use an Interlocked.CompareExchange to coordinate.  But that means that we always end up with two CompareExchange operations.  In the common path, there's no contention between these, and so we can avoid one of the CompareExchanges by first doing a normal read of the target field (we were already doing that read in one of the two cases, but we weren't taking advantage of it).

Commit migrated from https://github.com/dotnet/coreclr/commit/4410262505b5ebe308476116e9e833b210394638

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
index 6702010..ff9d3df 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
@@ -142,17 +142,22 @@ namespace System.Threading.Tasks.Sources
             // awaited twice concurrently), _continuationState might get erroneously overwritten.
             // To minimize the chances of that, we check preemptively whether _continuation
             // is already set to something other than the completion sentinel.
-            object currentContinuation = _continuation;
-            if (currentContinuation != null &&
-                !ReferenceEquals(currentContinuation, ManualResetValueTaskSourceCoreShared.s_sentinel))
+
+            object oldContinuation = _continuation;
+            if (oldContinuation == null)
             {
-                ManualResetValueTaskSourceCoreShared.ThrowInvalidOperationException();
+                _continuationState = state;
+                oldContinuation = Interlocked.CompareExchange(ref _continuation, continuation, null);
             }
-            _continuationState = state;
 
-            Action<object> oldContinuation = Interlocked.CompareExchange(ref _continuation, continuation, null);
             if (oldContinuation != null)
             {
+                // Operation already completed, so we need to queue the supplied callback.
+                if (!ReferenceEquals(oldContinuation, ManualResetValueTaskSourceCoreShared.s_sentinel))
+                {
+                    ManualResetValueTaskSourceCoreShared.ThrowInvalidOperationException();
+                }
+
                 switch (_capturedContext)
                 {
                     case null:
@@ -200,7 +205,7 @@ namespace System.Threading.Tasks.Sources
             }
             _completed = true;
 
-            if (Interlocked.CompareExchange(ref _continuation, ManualResetValueTaskSourceCoreShared.s_sentinel, null) != null)
+            if (_continuation != null || Interlocked.CompareExchange(ref _continuation, ManualResetValueTaskSourceCoreShared.s_sentinel, null) != null)
             {
                 if (_executionContext != null)
                 {