Reapply revert of #97227, fix Lock's waiter duration computation (#98876

)

* Reapply "Add an internal mode to `Lock` to have it use non-alertable waits (#97227)" (#98867)

This reverts commit f129701.

* Fix Lock's waiter duration computation

PR #97227 introduced a tick count masking issue where the stored waiter start time excludes the upper bit from the ushort tick count, but comparisons with it were not doing the appropriate masking. This was leading to a lock convoy on some heavily contended locks once in a while due to waiters incorrectly appearing to have waited for a long time.

Fixes #98021

* Fix wraparound issue

* Fix recording waiter start time

* Use a bit in the _state field instead

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

@@ -9,7 +9,7 @@ namespace System.Threading
     public abstract partial class WaitHandle
     {
         [MethodImpl(MethodImplOptions.InternalCall)]
-        private static extern int WaitOneCore(IntPtr waitHandle, int millisecondsTimeout);
+        private static extern int WaitOneCore(IntPtr waitHandle, int millisecondsTimeout, bool useTrivialWaits);
 
         private static unsafe int WaitMultipleIgnoringSyncContextCore(Span<IntPtr> waitHandles, bool waitAll, int millisecondsTimeout)
         {

src/coreclr/nativeaot/Common/src/System/Collections/Concurrent/ConcurrentUnifier.cs

@@ -65,7 +65,7 @@ internal abstract class ConcurrentUnifier<K, V>
     {
         protected ConcurrentUnifier()
         {
-            _lock = new Lock();
+            _lock = new Lock(useTrivialWaits: true);
             _container = new Container(this);
         }
 

src/coreclr/nativeaot/Common/src/System/Collections/Concurrent/ConcurrentUnifierW.cs

@@ -75,7 +75,7 @@ internal abstract class ConcurrentUnifierW<K, V>
     {
         protected ConcurrentUnifierW()
         {
-            _lock = new Lock();
+            _lock = new Lock(useTrivialWaits: true);
             _container = new Container(this);
         }
 

src/coreclr/nativeaot/Common/src/System/Collections/Concurrent/ConcurrentUnifierWKeyed.cs

@@ -84,7 +84,7 @@ internal abstract class ConcurrentUnifierWKeyed<K, V>
     {
         protected ConcurrentUnifierWKeyed()
         {
-            _lock = new Lock();
+            _lock = new Lock(useTrivialWaits: true);
             _container = new Container(this);
         }
 

src/coreclr/nativeaot/System.Private.CoreLib/src/CompatibilitySuppressions.xml

@@ -833,6 +833,10 @@
     <DiagnosticId>CP0002</DiagnosticId>
     <Target>M:System.Reflection.MethodBase.GetParametersAsSpan</Target>
   </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>M:System.Threading.Lock.#ctor(System.Boolean)</Target>
+  </Suppression>
   <Suppression>
     <DiagnosticId>CP0015</DiagnosticId>
     <Target>M:System.Diagnostics.Tracing.EventSource.Write``1(System.String,``0):[T:System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute]</Target>

src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/FrozenObjectHeapManager.cs

@@ -16,7 +16,7 @@ internal unsafe partial class FrozenObjectHeapManager
     {
         public static readonly FrozenObjectHeapManager Instance = new FrozenObjectHeapManager();
 
-        private readonly LowLevelLock m_Crst = new LowLevelLock();
+        private readonly Lock m_Crst = new Lock(useTrivialWaits: true);
         private FrozenObjectSegment m_CurrentSegment;
 
         // Default size to reserve for a frozen segment
@@ -34,9 +34,7 @@ internal unsafe partial class FrozenObjectHeapManager
         {
             HalfBakedObject* obj = null;
 
-            m_Crst.Acquire();
-
-            try
+            using (m_Crst.EnterScope())
             {
                 Debug.Assert(type != null);
                 // _ASSERT(FOH_COMMIT_SIZE >= MIN_OBJECT_SIZE);
@@ -84,10 +82,6 @@ internal unsafe partial class FrozenObjectHeapManager
                     Debug.Assert(obj != null);
                 }
             } // end of m_Crst lock
-            finally
-            {
-                m_Crst.Release();
-            }
 
             IntPtr result = (IntPtr)obj;
 

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/ClassConstructorRunner.cs

@@ -275,7 +275,7 @@ public static CctorHandle GetCctor(StaticClassConstructionContext* pContext)
 #if TARGET_WASM
                 if (s_cctorGlobalLock == null)
                 {
-                    Interlocked.CompareExchange(ref s_cctorGlobalLock, new Lock(), null);
+                    Interlocked.CompareExchange(ref s_cctorGlobalLock, new Lock(useTrivialWaits: true), null);
                 }
                 if (s_cctorArrays == null)
                 {
@@ -342,7 +342,7 @@ public static CctorHandle GetCctor(StaticClassConstructionContext* pContext)
 
                         Debug.Assert(resultArray[resultIndex]._pContext == default(StaticClassConstructionContext*));
                         resultArray[resultIndex]._pContext = pContext;
-                        resultArray[resultIndex].Lock = new Lock();
+                        resultArray[resultIndex].Lock = new Lock(useTrivialWaits: true);
                         s_count++;
                     }
 
@@ -489,7 +489,7 @@ public static CctorHandle GetCctorThatThreadIsBlockedOn(int managedThreadId)
         internal static void Initialize()
         {
             s_cctorArrays = new Cctor[10][];
-            s_cctorGlobalLock = new Lock();
+            s_cctorGlobalLock = new Lock(useTrivialWaits: true);
         }
 
         [Conditional("ENABLE_NOISY_CCTOR_LOG")]

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs

@@ -44,7 +44,7 @@ public abstract partial class ComWrappers
         private static readonly List<GCHandle> s_referenceTrackerNativeObjectWrapperCache = new List<GCHandle>();
 
         private readonly ConditionalWeakTable<object, ManagedObjectWrapperHolder> _ccwTable = new ConditionalWeakTable<object, ManagedObjectWrapperHolder>();
-        private readonly Lock _lock = new Lock();
+        private readonly Lock _lock = new Lock(useTrivialWaits: true);
         private readonly Dictionary<IntPtr, GCHandle> _rcwCache = new Dictionary<IntPtr, GCHandle>();
 
         internal static bool TryGetComInstanceForIID(object obj, Guid iid, out IntPtr unknown, out long wrapperId)

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

@@ -114,6 +114,7 @@ public unsafe bool Wait(int millisecondsTimeout, object? associatedObjectForMoni
                 success =
                     waiter.ev.WaitOneNoCheck(
                         millisecondsTimeout,
+                        false, // useTrivialWaits
                         associatedObjectForMonitorWait,
                         associatedObjectForMonitorWait != null
                             ? NativeRuntimeEventSource.WaitHandleWaitSourceMap.MonitorWait

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Lock.NativeAot.cs

@@ -92,6 +92,18 @@ internal void Reenter(uint previousRecursionCount)
             _recursionCount = previousRecursionCount;
         }
 
+        private static bool IsFullyInitialized
+        {
+            get
+            {
+                // If NativeRuntimeEventSource is already being class-constructed by this thread earlier in the stack, Log can
+                // be null. This property is used to avoid going down the wait path in that case to avoid null checks in several
+                // other places.
+                Debug.Assert((StaticsInitializationStage)s_staticsInitializationStage == StaticsInitializationStage.Complete);
+                return NativeRuntimeEventSource.Log != null;
+            }
+        }
+
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private TryLockResult LazyInitializeOrEnter()
         {
@@ -101,6 +113,10 @@ private TryLockResult LazyInitializeOrEnter()
                 case StaticsInitializationStage.Complete:
                     if (_spinCount == SpinCountNotInitialized)
                     {
+                        if (!IsFullyInitialized)
+                        {
+                            goto case StaticsInitializationStage.Started;
+                        }
                         _spinCount = s_maxSpinCount;
                     }
                     return TryLockResult.Spin;
@@ -121,7 +137,7 @@ private TryLockResult LazyInitializeOrEnter()
                         }
 
                         stage = (StaticsInitializationStage)Volatile.Read(ref s_staticsInitializationStage);
-                        if (stage == StaticsInitializationStage.Complete)
+                        if (stage == StaticsInitializationStage.Complete && IsFullyInitialized)
                         {
                             goto case StaticsInitializationStage.Complete;
                         }
@@ -166,14 +182,17 @@ private static bool TryInitializeStatics()
                     return true;
             }
 
+            bool isFullyInitialized;
             try
             {
                 s_isSingleProcessor = Environment.IsSingleProcessor;
                 s_maxSpinCount = DetermineMaxSpinCount();
                 s_minSpinCount = DetermineMinSpinCount();
 
-                // Also initialize some types that are used later to prevent potential class construction cycles
-                _ = NativeRuntimeEventSource.Log;
+                // Also initialize some types that are used later to prevent potential class construction cycles. If
+                // NativeRuntimeEventSource is already being class-constructed by this thread earlier in the stack, Log can be
+                // null. Avoid going down the wait path in that case to avoid null checks in several other places.
+                isFullyInitialized = NativeRuntimeEventSource.Log != null;
             }
             catch
             {
@@ -182,20 +201,24 @@ private static bool TryInitializeStatics()
             }
 
             Volatile.Write(ref s_staticsInitializationStage, (int)StaticsInitializationStage.Complete);
-            return true;
+            return isFullyInitialized;
         }
 
         // Returns false until the static variable is lazy-initialized
         internal static bool IsSingleProcessor => s_isSingleProcessor;
 
-        // Used to transfer the state when inflating thin locks
-        internal void InitializeLocked(int managedThreadId, uint recursionCount)
+        // Used to transfer the state when inflating thin locks. The lock is considered unlocked if managedThreadId is zero, and
+        // locked otherwise.
+        internal void ResetForMonitor(int managedThreadId, uint recursionCount)
         {
             Debug.Assert(recursionCount == 0 || managedThreadId != 0);
+            Debug.Assert(!new State(this).UseTrivialWaits);
 
             _state = managedThreadId == 0 ? State.InitialStateValue : State.LockedStateValue;
             _owningThreadId = (uint)managedThreadId;
             _recursionCount = recursionCount;
+
+            Debug.Assert(!new State(this).UseTrivialWaits);
         }
 
         internal struct ThreadId

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

@@ -65,7 +65,7 @@ internal static class SyncTable
         /// <summary>
         /// Protects all mutable operations on s_entries, s_freeEntryList, s_unusedEntryIndex. Also protects growing the table.
         /// </summary>
-        internal static readonly Lock s_lock = new Lock();
+        internal static readonly Lock s_lock = new Lock(useTrivialWaits: true);
 
         /// <summary>
         /// The dynamically growing array of sync entries.
@@ -274,7 +274,7 @@ public static void MoveThinLockToNewEntry(int syncIndex, int threadId, uint recu
             Debug.Assert(s_lock.IsHeldByCurrentThread);
             Debug.Assert((0 < syncIndex) && (syncIndex < s_unusedEntryIndex));
 
-            s_entries[syncIndex].Lock.InitializeLocked(threadId, recursionLevel);
+            s_entries[syncIndex].Lock.ResetForMonitor(threadId, recursionLevel);
         }
 
         /// <summary>

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.Windows.cs

@@ -167,7 +167,7 @@ private bool JoinInternal(int millisecondsTimeout)
                 }
                 else
                 {
-                    result = WaitHandle.WaitOneCore(waitHandle.DangerousGetHandle(), millisecondsTimeout);
+                    result = WaitHandle.WaitOneCore(waitHandle.DangerousGetHandle(), millisecondsTimeout, useTrivialWaits: false);
                 }
 
                 return result == (int)Interop.Kernel32.WAIT_OBJECT_0;

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.cs

@@ -31,7 +31,7 @@ public sealed partial class Thread
         private Exception? _startException;
 
         // Protects starting the thread and setting its priority
-        private Lock _lock = new Lock();
+        private Lock _lock = new Lock(useTrivialWaits: true);
 
         // This is used for a quick check on thread pool threads after running a work item to determine if the name, background
         // state, or priority were changed by the work item, and if so to reset it. Other threads may also change some of those,

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Type.NativeAot.cs

@@ -31,15 +31,14 @@ internal static unsafe RuntimeType GetTypeFromMethodTable(MethodTable* pMT)
 
         private static class AllocationLockHolder
         {
-            public static LowLevelLock AllocationLock = new LowLevelLock();
+            public static Lock AllocationLock = new Lock(useTrivialWaits: true);
         }
 
         [MethodImpl(MethodImplOptions.NoInlining)]
         private static unsafe RuntimeType GetTypeFromMethodTableSlow(MethodTable* pMT)
         {
             // Allocate and set the RuntimeType under a lock - there's no way to free it if there is a race.
-            AllocationLockHolder.AllocationLock.Acquire();
-            try
+            using (AllocationLockHolder.AllocationLock.EnterScope())
             {
                 ref RuntimeType? runtimeTypeCache = ref Unsafe.AsRef<RuntimeType?>(pMT->WritableData);
                 if (runtimeTypeCache != null)
@@ -55,10 +54,6 @@ private static unsafe RuntimeType GetTypeFromMethodTableSlow(MethodTable* pMT)
 
                 return type;
             }
-            finally
-            {
-                AllocationLockHolder.AllocationLock.Release();
-            }
         }
 
         //

src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.ConstructedGenericsRegistration.cs

@@ -24,7 +24,7 @@ internal struct DynamicGenericsRegistrationData
         }
 
         // To keep the synchronization simple, we execute all dynamic generic type registration/lookups under a global lock
-        private Lock _dynamicGenericsLock = new Lock();
+        private Lock _dynamicGenericsLock = new Lock(useTrivialWaits: true);
 
         internal void RegisterDynamicGenericTypesAndMethods(DynamicGenericsRegistrationData registrationData)
         {

src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.StaticsLookup.cs

@@ -15,7 +15,7 @@ namespace Internal.Runtime.TypeLoader
     public sealed partial class TypeLoaderEnvironment
     {
         // To keep the synchronization simple, we execute all TLS registration/lookups under a global lock
-        private Lock _threadStaticsLock = new Lock();
+        private Lock _threadStaticsLock = new Lock(useTrivialWaits: true);
 
         // Counter to keep track of generated offsets for TLS cells of dynamic types;
         private LowLevelDictionary<IntPtr, uint> _maxThreadLocalIndex = new LowLevelDictionary<IntPtr, uint>();

src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.cs

@@ -145,7 +145,7 @@ internal static void Initialize()
         }
 
         // To keep the synchronization simple, we execute all type loading under a global lock
-        private Lock _typeLoaderLock = new Lock();
+        private Lock _typeLoaderLock = new Lock(useTrivialWaits: true);
 
         public void VerifyTypeLoaderLockHeld()
         {

src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeSystemContextFactory.cs

@@ -18,7 +18,7 @@ public static class TypeSystemContextFactory
         // This allows us to avoid recreating the type resolution context again and again, but still allows it to go away once the types are no longer being built
         private static GCHandle s_cachedContext = GCHandle.Alloc(null, GCHandleType.Weak);
 
-        private static Lock s_lock = new Lock();
+        private static Lock s_lock = new Lock(useTrivialWaits: true);
 
         public static TypeSystemContext Create()
         {

src/coreclr/vm/comwaithandle.cpp

@@ -16,7 +16,7 @@
 #include "excep.h"
 #include "comwaithandle.h"
 
-FCIMPL2(INT32, WaitHandleNative::CorWaitOneNative, HANDLE handle, INT32 timeout)
+FCIMPL3(INT32, WaitHandleNative::CorWaitOneNative, HANDLE handle, INT32 timeout, CLR_BOOL useTrivialWaits)
 {
     FCALL_CONTRACT;
 
@@ -28,7 +28,8 @@ FCIMPL2(INT32, WaitHandleNative::CorWaitOneNative, HANDLE handle, INT32 timeout)
 
     Thread* pThread = GET_THREAD();
 
-    retVal = pThread->DoAppropriateWait(1, &handle, TRUE, timeout, (WaitMode)(WaitMode_Alertable | WaitMode_IgnoreSyncCtx));
+    WaitMode waitMode = (WaitMode)((!useTrivialWaits ? WaitMode_Alertable : WaitMode_None) | WaitMode_IgnoreSyncCtx);
+    retVal = pThread->DoAppropriateWait(1, &handle, TRUE, timeout, waitMode);
 
     HELPER_METHOD_FRAME_END();
     return retVal;

src/coreclr/vm/comwaithandle.h

@@ -18,7 +18,7 @@
 class WaitHandleNative
 {
 public:
-    static FCDECL2(INT32, CorWaitOneNative, HANDLE handle, INT32 timeout);
+    static FCDECL3(INT32, CorWaitOneNative, HANDLE handle, INT32 timeout, CLR_BOOL useTrivialWaits);
     static FCDECL4(INT32, CorWaitMultipleNative, HANDLE *handleArray, INT32 numHandles, CLR_BOOL waitForAll, INT32 timeout);
     static FCDECL3(INT32, CorSignalAndWaitOneNative, HANDLE waitHandleSignalUNSAFE, HANDLE waitHandleWaitUNSAFE, INT32 timeout);
 };