Remove Monitor asm helpers (#14146)

- Removed asm helpers on Windows and used portable C++ helpers instead
- Rearranged fast path code to improve them a bit and match the asm more closely

Perf:
- The asm helpers are a bit faster. The code generated for the portable helpers is almost the same now, the remaining differences are:
  - There were some layout issues where hot paths were in the wrong place and return paths were not cloned. Instrumenting some of the tests below with PGO on x64 resolved all of the layout issues. I couldn't get PGO instrumentation to work on x86 but I imagine it would be the same there.
  - Register usage
    - x64: All of the Enter functions are using one or two (TryEnter is using two) callee-saved registers for no apparent reason, forcing them to be saved and restored. r10 and r11 seem to be available but they're not being used.
    - x86: Similarly to x64, the compiled functions are pushing and popping 2-3 additional registers in the hottest fast paths.
    - I believe this is the main remaining gap and PGO is not helping with this
- On Linux, perf is >= before for the most part
- Perf tests used for below are updated in PR https://github.com/dotnet/coreclr/pull/13670

1 files changed, 100 insertions, 127 deletions

diff --git a/src/vm/syncblk.inl b/src/vm/syncblk.inl
index cb6b280228..376cd4c2e7 100644
--- a/src/vm/syncblk.inl
+++ b/src/vm/syncblk.inl
@@ -8,157 +8,141 @@
 
 #ifndef DACCESS_COMPILE
 
-FORCEINLINE AwareLock::EnterHelperResult AwareLock::EnterHelper(Thread* pCurThread)
+FORCEINLINE bool AwareLock::SpinWaitAndBackOffBeforeOperation(DWORD *spinCountRef)
 {
-    CONTRACTL {
+    CONTRACTL{
         SO_TOLERANT;
         NOTHROW;
         GC_NOTRIGGER;
-        MODE_ANY;
+        MODE_COOPERATIVE;
     } CONTRACTL_END;
 
-    for (;;)
+    _ASSERTE(spinCountRef != nullptr);
+    DWORD &spinCount = *spinCountRef;
+    _ASSERTE(g_SystemInfo.dwNumberOfProcessors != 1);
+
+    if (spinCount > g_SpinConstants.dwMaximumDuration)
     {
-        LONG state = m_MonitorHeld.LoadWithoutBarrier();
+        return false;
+    }
 
-        if (state == 0)
-        {
-            if (InterlockedCompareExchangeAcquire((LONG*)&m_MonitorHeld, 1, 0) == 0)
-            {
-                m_HoldingThread = pCurThread;
-                m_Recursion = 1;
-                pCurThread->IncLockCount();
-                return AwareLock::EnterHelperResult_Entered;
-            }
-        }
-        else
-        {
-            if (GetOwningThread() == pCurThread) /* monitor is held, but it could be a recursive case */
-            {
-                m_Recursion++;
-                return AwareLock::EnterHelperResult_Entered;
-            }
+    for (DWORD i = 0; i < spinCount; i++)
+    {
+        YieldProcessor();
+    }
+
+    spinCount *= g_SpinConstants.dwBackoffFactor;
+    return true;
+}
+
+FORCEINLINE bool AwareLock::EnterHelper(Thread* pCurThread, bool checkRecursiveCase)
+{
+    CONTRACTL{
+        SO_TOLERANT;
+        NOTHROW;
+        GC_NOTRIGGER;
+        MODE_ANY;
+    } CONTRACTL_END;
 
-            return AwareLock::EnterHelperResult_Contention;
+    LONG state = m_MonitorHeld.LoadWithoutBarrier();
+    if (state == 0)
+    {
+        if (InterlockedCompareExchangeAcquire((LONG*)&m_MonitorHeld, 1, 0) == 0)
+        {
+            m_HoldingThread = pCurThread;
+            m_Recursion = 1;
+            pCurThread->IncLockCount();
+            return true;
         }
     }
+    else if (checkRecursiveCase && GetOwningThread() == pCurThread) /* monitor is held, but it could be a recursive case */
+    {
+        m_Recursion++;
+        return true;
+    }
+    return false;
 }
 
 FORCEINLINE AwareLock::EnterHelperResult ObjHeader::EnterObjMonitorHelper(Thread* pCurThread)
 {
-    CONTRACTL {
+    CONTRACTL{
         SO_TOLERANT;
         NOTHROW;
         GC_NOTRIGGER;
         MODE_COOPERATIVE;
     } CONTRACTL_END;
 
-    DWORD tid = pCurThread->GetThreadId();
+    LONG oldValue = m_SyncBlockValue.LoadWithoutBarrier();
 
-    LONG oldvalue = m_SyncBlockValue.LoadWithoutBarrier();
-            
-    if ((oldvalue & (BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX + 
-                    BIT_SBLK_SPIN_LOCK + 
-                    SBLK_MASK_LOCK_THREADID + 
-                    SBLK_MASK_LOCK_RECLEVEL)) == 0)
+    if ((oldValue & (BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX +
+                     BIT_SBLK_SPIN_LOCK +
+                     SBLK_MASK_LOCK_THREADID +
+                     SBLK_MASK_LOCK_RECLEVEL)) == 0)
     {
+        DWORD tid = pCurThread->GetThreadId();
         if (tid > SBLK_MASK_LOCK_THREADID)
         {
             return AwareLock::EnterHelperResult_UseSlowPath;
         }
 
-        LONG newvalue = oldvalue | tid;
-        if (InterlockedCompareExchangeAcquire((LONG*)&m_SyncBlockValue, newvalue, oldvalue) == oldvalue)
+        LONG newValue = oldValue | tid;
+        if (InterlockedCompareExchangeAcquire((LONG*)&m_SyncBlockValue, newValue, oldValue) == oldValue)
         {
             pCurThread->IncLockCount();
             return AwareLock::EnterHelperResult_Entered;
         }
-    }
-    else
-    if (oldvalue & BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX)
-    {
-         // If we have a hash code already, we need to create a sync block
-         if (oldvalue & BIT_SBLK_IS_HASHCODE)
-         {
-             return AwareLock::EnterHelperResult_UseSlowPath;
-         }
-
-         SyncBlock *syncBlock = g_pSyncTable [oldvalue & MASK_SYNCBLOCKINDEX].m_SyncBlock;
-         _ASSERTE(syncBlock != NULL);
 
-         return syncBlock->m_Monitor.EnterHelper(pCurThread);
+        return AwareLock::EnterHelperResult_Contention;
     }
-    else
+
+    if (oldValue & BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX)
     {
-        // The header is transitioning - treat this as if the lock was taken
-        if (oldvalue & BIT_SBLK_SPIN_LOCK)
+        // If we have a hash code already, we need to create a sync block
+        if (oldValue & BIT_SBLK_IS_HASHCODE)
         {
-            return AwareLock::EnterHelperResult_Contention;
+            return AwareLock::EnterHelperResult_UseSlowPath;
         }
 
-        // Here we know we have the "thin lock" layout, but the lock is not free.
-        // It could still be the recursion case - compare the thread id to check
-        if (tid == (DWORD) (oldvalue & SBLK_MASK_LOCK_THREADID))
+        SyncBlock *syncBlock = g_pSyncTable[oldValue & MASK_SYNCBLOCKINDEX].m_SyncBlock;
+        _ASSERTE(syncBlock != NULL);
+        if (syncBlock->m_Monitor.EnterHelper(pCurThread, true /* checkRecursiveCase */))
         {
-            // Ok, the thread id matches, it's the recursion case.
-            // Bump up the recursion level and check for overflow
-            LONG newvalue = oldvalue + SBLK_LOCK_RECLEVEL_INC;
-
-            if ((newvalue & SBLK_MASK_LOCK_RECLEVEL) == 0)
-            {
-                return AwareLock::EnterHelperResult_UseSlowPath;
-            }
-
-            if (InterlockedCompareExchangeAcquire((LONG*)&m_SyncBlockValue, newvalue, oldvalue) == oldvalue)
-            {
-                return AwareLock::EnterHelperResult_Entered;
-            }
+            return AwareLock::EnterHelperResult_Entered;
         }
-    }
 
-    return AwareLock::EnterHelperResult_Contention;
-}
+        return AwareLock::EnterHelperResult_Contention;
+    }
 
-inline AwareLock::EnterHelperResult ObjHeader::EnterObjMonitorHelperSpin(Thread* pCurThread)
-{
-    CONTRACTL {
-        SO_TOLERANT;
-        NOTHROW;
-        GC_NOTRIGGER;
-        MODE_COOPERATIVE;
-    } CONTRACTL_END;
+    // The header is transitioning - treat this as if the lock was taken
+    if (oldValue & BIT_SBLK_SPIN_LOCK)
+    {
+        return AwareLock::EnterHelperResult_Contention;
+    }
 
-    if (1 == g_SystemInfo.dwNumberOfProcessors)
+    // Here we know we have the "thin lock" layout, but the lock is not free.
+    // It could still be the recursion case - compare the thread id to check
+    if (pCurThread->GetThreadId() != (DWORD)(oldValue & SBLK_MASK_LOCK_THREADID))
     {
         return AwareLock::EnterHelperResult_Contention;
     }
 
-    DWORD spincount = g_SpinConstants.dwInitialDuration;
+    // Ok, the thread id matches, it's the recursion case.
+    // Bump up the recursion level and check for overflow
+    LONG newValue = oldValue + SBLK_LOCK_RECLEVEL_INC;
 
-    for (;;)
+    if ((newValue & SBLK_MASK_LOCK_RECLEVEL) == 0)
     {
-        //
-        // exponential backoff
-        //
-        for (DWORD i = 0; i < spincount; i++)
-        {
-            YieldProcessor();
-        }
-
-        AwareLock::EnterHelperResult result = EnterObjMonitorHelper(pCurThread);
-        if (result != AwareLock::EnterHelperResult_Contention)
-        {
-            return result;
-        }
+        return AwareLock::EnterHelperResult_UseSlowPath;
+    }
 
-        spincount *= g_SpinConstants.dwBackoffFactor;
-        if (spincount > g_SpinConstants.dwMaximumDuration)
-        {
-            break;
-        }
+    if (InterlockedCompareExchangeAcquire((LONG*)&m_SyncBlockValue, newValue, oldValue) == oldValue)
+    {
+        return AwareLock::EnterHelperResult_Entered;
     }
 
-    return AwareLock::EnterHelperResult_Contention;
+    // 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 AwareLock::EnterHelperResult_UseSlowPath;
 }
 
 // Helper encapsulating the core logic for releasing monitor. Returns what kind of 
@@ -185,31 +169,20 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre
     pCurThread->m_pTrackSync->LeaveSync(caller, this);
 #endif
 
-    if (--m_Recursion != 0)
-    {
-        return AwareLock::LeaveHelperAction_None;
-    }
-
-    m_HoldingThread->DecLockCount();
-    m_HoldingThread = NULL;
-
-    for (;;)
+    if (--m_Recursion == 0)
     {
-        // Read existing lock state
-        LONG state = m_MonitorHeld.LoadWithoutBarrier();
+        m_HoldingThread->DecLockCount();
+        m_HoldingThread = NULL;
 
         // Clear lock bit.
-        if (InterlockedCompareExchangeRelease((LONG*)&m_MonitorHeld, state - 1, state) == state)
+        LONG state = InterlockedDecrementRelease((LONG*)&m_MonitorHeld);
+
+        // If wait count is non-zero on successful clear, we must signal the event.
+        if (state & ~1)
         {
-            // If wait count is non-zero on successful clear, we must signal the event.
-            if (state & ~1)
-            {
-                return AwareLock::LeaveHelperAction_Signal;
-            }
-            break;
+            return AwareLock::LeaveHelperAction_Signal;
         }
     }
-
     return AwareLock::LeaveHelperAction_None;
 }
 
@@ -234,34 +207,34 @@ FORCEINLINE AwareLock::LeaveHelperAction ObjHeader::LeaveObjMonitorHelper(Thread
             return AwareLock::LeaveHelperAction_Error;                
         }
 
-        if (syncBlockValue & SBLK_MASK_LOCK_RECLEVEL)
+        if (!(syncBlockValue & SBLK_MASK_LOCK_RECLEVEL))
         {
-            // recursion and ThinLock
-            DWORD newValue = syncBlockValue - SBLK_LOCK_RECLEVEL_INC;
+            // We are leaving the lock
+            DWORD newValue = (syncBlockValue & (~SBLK_MASK_LOCK_THREADID));
             if (InterlockedCompareExchangeRelease((LONG*)&m_SyncBlockValue, newValue, syncBlockValue) != (LONG)syncBlockValue)
             {
                 return AwareLock::LeaveHelperAction_Yield;
             }
+            pCurThread->DecLockCount();
         }
         else
         {
-            // We are leaving the lock
-            DWORD newValue = (syncBlockValue & (~SBLK_MASK_LOCK_THREADID));
+            // recursion and ThinLock
+            DWORD newValue = syncBlockValue - SBLK_LOCK_RECLEVEL_INC;
             if (InterlockedCompareExchangeRelease((LONG*)&m_SyncBlockValue, newValue, syncBlockValue) != (LONG)syncBlockValue)
             {
                 return AwareLock::LeaveHelperAction_Yield;
             }
-            pCurThread->DecLockCount();
         }
 
         return AwareLock::LeaveHelperAction_None;
     }
 
-    if ((syncBlockValue & (BIT_SBLK_SPIN_LOCK + BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX + BIT_SBLK_IS_HASHCODE)) == BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX)
+    if ((syncBlockValue & (BIT_SBLK_SPIN_LOCK + BIT_SBLK_IS_HASHCODE)) == 0)
     {
-        SyncBlock *syncBlock = g_pSyncTable [syncBlockValue & MASK_SYNCBLOCKINDEX].m_SyncBlock;
+        _ASSERTE((syncBlockValue & BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX) != 0);
+        SyncBlock *syncBlock = g_pSyncTable[syncBlockValue & MASK_SYNCBLOCKINDEX].m_SyncBlock;
         _ASSERTE(syncBlock != NULL);
-
         return syncBlock->m_Monitor.LeaveHelper(pCurThread);
     }