Refactor lock upgrade logic (#122571)

Author: jkoritzinsky

src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
@@ -127,6 +128,26 @@ public static int CompareExchange(ref int location1, int value, int comparand)
         [MethodImpl(MethodImplOptions.InternalCall)]
         private static extern int CompareExchange32(ref int location1, int value, int comparand);
 
+        // This is used internally in cases where having a managed
+        // ref to the location is unsafe (Ex: it is the object header of a pinned object).
+        // The intrinsic expansion for this overload is exactly the same as for the `ref int`
+        // variant and will go on the same path since expansion is triggered by the name and
+        // return type of the method.
+        // The important part is avoiding `ref *location` that is reported as byref to the GC.
+        [Intrinsic]
+        internal static unsafe int CompareExchange(int* location1, int value, int comparand)
+        {
+#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64
+            return CompareExchange(location1, value, comparand); // Must expand intrinsic
+#else
+            Debug.Assert(location1 != null);
+            return CompareExchange32Pointer(location1, value, comparand);
+#endif
+        }
+
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        private static unsafe extern int CompareExchange32Pointer(int* location1, int value, int comparand);
+
         /// <summary>Compares two 64-bit signed integers for equality and, if they are equal, replaces the first value.</summary>
         /// <param name="location1">The destination, whose value is compared with <paramref name="comparand"/> and possibly replaced.</param>
         /// <param name="value">The value that replaces the destination value if the comparison results in equality.</param>

src/coreclr/System.Private.CoreLib/src/System/Threading/ObjectHeader.CoreCLR.cs

@@ -150,7 +150,7 @@ private static unsafe HeaderLockResult TryAcquireThinLockSpin(object obj)
                             int newBits = oldBits + SBLK_LOCK_RECLEVEL_INC;
                             if ((newBits & SBLK_MASK_LOCK_RECLEVEL) != 0)
                             {
-                                if (Interlocked.CompareExchange(ref *pHeader, newBits, oldBits) == oldBits)
+                                if (Interlocked.CompareExchange(pHeader, newBits, oldBits) == oldBits)
                                 {
                                     return HeaderLockResult.Success;
                                 }
@@ -170,7 +170,7 @@ private static unsafe HeaderLockResult TryAcquireThinLockSpin(object obj)
                         else if ((oldBits & SBLK_MASK_LOCK_THREADID) == 0)
                         {
                             int newBits = oldBits | currentThreadID;
-                            if (Interlocked.CompareExchange(ref *pHeader, newBits, oldBits) == oldBits)
+                            if (Interlocked.CompareExchange(pHeader, newBits, oldBits) == oldBits)
                             {
                                 return HeaderLockResult.Success;
                             }

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs

@@ -24,13 +24,12 @@ public static int CompareExchange(ref int location1, int value, int comparand)
 #endif
         }
 
-        // This is used internally by NativeAOT runtime in cases where having a managed
-        // ref to the location is unsafe (Ex: it is the syncblock of a pinned object).
+        // This is used internally in cases where having a managed
+        // ref to the location is unsafe (Ex: it is the object header of a pinned object).
         // The intrinsic expansion for this overload is exactly the same as for the `ref int`
         // variant and will go on the same path since expansion is triggered by the name and
         // return type of the method.
-        // The important part is avoiding `ref *location` in the unexpanded scenario, like
-        // in a case when compiling the Debug flavor of the app.
+        // The important part is avoiding `ref *location` that is reported as byref to the GC.
         [Intrinsic]
         internal static unsafe int CompareExchange(int* location1, int value, int comparand)
         {

src/coreclr/vm/ecalllist.h

@@ -323,6 +323,7 @@ FCFuncStart(gInterlockedFuncs)
     FCFuncElement("Exchange64", COMInterlocked::Exchange64)
     FCFuncElement("ExchangeObject", COMInterlocked::ExchangeObject)
     FCFuncElement("CompareExchange32", COMInterlocked::CompareExchange32)
+    FCFuncElement("CompareExchange32Pointer", COMInterlocked::CompareExchange32)
     FCFuncElement("CompareExchange64", COMInterlocked::CompareExchange64)
     FCFuncElement("CompareExchangeObject", COMInterlocked::CompareExchangeObject)
     FCFuncElement("ExchangeAdd32", COMInterlocked::ExchangeAdd32)

src/coreclr/vm/syncblk.cpp

@@ -1737,6 +1737,25 @@ OBJECTHANDLE SyncBlock::GetOrCreateLock(OBJECTREF lockObj)
     // Create the handle here.
     OBJECTHANDLEHolder lockHandle = GetAppDomain()->CreateHandle(lockObj);
 
+    if (TryUpgradeThinLockToFullLock(lockHandle.GetValue()))
+    {
+        // Our lock instance is the one in the sync block now.
+        return lockHandle.Extract();
+    }
+
+    return VolatileLoad(&m_Lock);
+}
+
+bool SyncBlock::TryUpgradeThinLockToFullLock(OBJECTHANDLE lockHandle)
+{
+    CONTRACTL
+    {
+        GC_TRIGGERS;
+        THROWS;
+        MODE_COOPERATIVE;
+    }
+    CONTRACTL_END;
+
     // Switch to preemptive so we can grab the spin-lock.
     // Use the NO_DTOR version so we don't do a coop->preemptive->coop transition on return.
     GCX_PREEMP_NO_DTOR();
@@ -1747,10 +1766,9 @@ OBJECTHANDLE SyncBlock::GetOrCreateLock(OBJECTREF lockObj)
     GCX_PREEMP_NO_DTOR_END();
 
     // Check again now that we hold the spin-lock
-    existingLock = m_Lock;
-    if (existingLock != (OBJECTHANDLE)NULL)
+    if (m_Lock != (OBJECTHANDLE)NULL)
     {
-        return existingLock;
+        return false;
     }
 
     // We need to create a new lock
@@ -1761,32 +1779,31 @@ OBJECTHANDLE SyncBlock::GetOrCreateLock(OBJECTREF lockObj)
 
     if (thinLock != 0)
     {
-
-        // We have thin-lock info that needs to be transferred to the lock object.
         DWORD lockThreadId = thinLock & SBLK_MASK_LOCK_THREADID;
         DWORD recursionLevel = (thinLock & SBLK_MASK_LOCK_RECLEVEL) >> SBLK_RECLEVEL_SHIFT;
         _ASSERTE(lockThreadId != 0);
         PREPARE_NONVIRTUAL_CALLSITE(METHOD__LOCK__INITIALIZE_FOR_MONITOR);
+
+        // We have thin-lock info that needs to be transferred to the lock object.
+        OBJECTREF lockObj = ObjectFromHandle(lockHandle);
+
         DECLARE_ARGHOLDER_ARRAY(args, 3);
-        args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(ObjectFromHandle(lockHandle));
+        args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(lockObj);
         args[ARGNUM_1] = DWORD_TO_ARGHOLDER(lockThreadId);
         args[ARGNUM_2] = DWORD_TO_ARGHOLDER(recursionLevel);
         CALL_MANAGED_METHOD_NORET(args);
     }
 
-    VolatileStore(&m_Lock, lockHandle.GetValue());
-
-    // Our lock instance is in the sync block now.
-    // Don't release it.
-    lockHandle.SuppressRelease();
-
-    // Also, clear the thin lock info.
+    VolatileStore(&m_Lock, lockHandle);
+    // Clear the thin lock info.
     // It won't be used any more, but it will look out of date.
     // Only clear the relevant bits, as the spin-lock bit is used to lock this method.
     // That bit will be reset upon return.
     m_thinLock.StoreWithoutBarrier(m_thinLock.LoadWithoutBarrier() & ~((SBLK_MASK_LOCK_THREADID) | (SBLK_MASK_LOCK_RECLEVEL)));
 
-    return lockHandle;
+    // Our lock instance is in the sync block now.
+    // Don't release it.
+    return true;
 }
 #endif // !DACCESS_COMPILE
 

src/coreclr/vm/syncblk.h

@@ -592,6 +592,8 @@ class SyncBlock
     private:
     void InitializeThinLock(DWORD recursionLevel, DWORD threadId);
 
+    bool TryUpgradeThinLockToFullLock(OBJECTHANDLE lockHandle);
+
     friend struct ::cdac_data<SyncBlock>;
 };
 

src/coreclr/vm/syncblk.inl

@@ -74,9 +74,8 @@ FORCEINLINE ObjHeader::HeaderLockResult ObjHeader::AcquireHeaderThinLock(DWORD t
         return HeaderLockResult::Success;
     }
 
-    // Use the slow path instead of spinning. The compare-exchange above would not fail often, and it's not worth forcing the
-    // spin loop that typically follows the call to this function to check the recursive case, so just bail to the slow path.
-    return HeaderLockResult::UseSlowPath;
+    // We failed to acquire because someone touched other bits in the header.
+    return HeaderLockResult::Failure;
 }
 
 // Helper encapsulating the core logic for releasing monitor. Returns what kind of