Use a bit in the _state field instead

Author: Koundinya Veluri

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

@@ -207,14 +207,18 @@ private static bool TryInitializeStatics()
         // 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

@@ -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/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs

@@ -22,7 +22,7 @@ public sealed partial class Lock
         private const short DefaultMaxSpinCount = 22;
         private const short DefaultAdaptiveSpinPeriod = 100;
         private const short SpinSleep0Threshold = 10;
-        private const int MaxDurationMsForPreemptingWaiters = 100;
+        private const ushort MaxDurationMsForPreemptingWaiters = 100;
 
         private static long s_contentionCount;
 
@@ -38,10 +38,7 @@ public sealed partial class Lock
         private uint _state; // see State for layout
         private uint _recursionCount;
         private short _spinCount;
-
-        // The lowest bit is a flag, when set it indicates that the lock should use trivial waits
-        private ushort _waiterStartTimeMsAndFlags;
-
+        private ushort _waiterStartTimeMs;
         private AutoResetEvent? _waitEvent;
 
 #if NATIVEAOT // The method needs to be public in NativeAOT so that other private libraries can access it
@@ -51,10 +48,7 @@ internal Lock(bool useTrivialWaits)
 #endif
             : this()
         {
-            if (useTrivialWaits)
-            {
-                _waiterStartTimeMsAndFlags = 1;
-            }
+            State.InitializeUseTrivialWaits(this, useTrivialWaits);
         }
 
         /// <summary>
@@ -488,7 +482,7 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)
                 int remainingTimeoutMs = timeoutMs;
                 while (true)
                 {
-                    if (!waitEvent.WaitOneNoCheck(remainingTimeoutMs, UseTrivialWaits))
+                    if (!waitEvent.WaitOneNoCheck(remainingTimeoutMs, new State(this).UseTrivialWaits))
                     {
                         break;
                     }
@@ -567,43 +561,18 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)
             return new ThreadId(0);
         }
 
-        // Trivial waits are:
-        // - Not interruptible by Thread.Interrupt
-        // - Don't allow reentrance through APCs or message pumping
-        // - Not forwarded to SynchronizationContext wait overrides
-        private bool UseTrivialWaits => (_waiterStartTimeMsAndFlags & 1) != 0;
-
-        // The stored waiter start time (ms) only includes the lower 15 bits since the field is also used for a flag. This mask
-        // should be used when recording and comparing waiter times.
-        private const int WaiterTimeMsMask = 0x7fff;
-
-        // Only the lower 15 bits are stored/retrieved. Callers should use WaiterTimeMsMask when recording and comparing waiter
-        // times.
-        private int WaiterStartTimeMs
-        {
-            get => _waiterStartTimeMsAndFlags >> 1;
-            set
-            {
-                Debug.Assert((value & WaiterTimeMsMask) == value);
-                _waiterStartTimeMsAndFlags = (ushort)((value << 1) | (_waiterStartTimeMsAndFlags & 1));
-            }
-        }
-
-        private void ResetWaiterStartTime() => WaiterStartTimeMs = 0;
+        private void ResetWaiterStartTime() => _waiterStartTimeMs = 0;
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private void RecordWaiterStartTime()
         {
-            int currentTimeMs = Environment.TickCount & WaiterTimeMsMask;
-
-            // Don't record zero, that value is reserved for indicating that a time is not recorded
+            ushort currentTimeMs = (ushort)Environment.TickCount;
             if (currentTimeMs == 0)
             {
-                currentTimeMs = (currentTimeMs - 1) & WaiterTimeMsMask;
+                // Don't record zero, that value is reserved for indicating that a time is not recorded
+                currentTimeMs--;
             }
-
-            Debug.Assert(currentTimeMs != 0);
-            WaiterStartTimeMs = currentTimeMs;
+            _waiterStartTimeMs = currentTimeMs;
         }
 
         private bool ShouldStopPreemptingWaiters
@@ -612,10 +581,10 @@ private bool ShouldStopPreemptingWaiters
             get
             {
                 // If the recorded time is zero, a time has not been recorded yet
-                int waiterStartTimeMs = WaiterStartTimeMs;
+                ushort waiterStartTimeMs = _waiterStartTimeMs;
                 return
                     waiterStartTimeMs != 0 &&
-                    ((Environment.TickCount - waiterStartTimeMs) & WaiterTimeMsMask) >= MaxDurationMsForPreemptingWaiters;
+                    (ushort)(Environment.TickCount - waiterStartTimeMs) >= MaxDurationMsForPreemptingWaiters;
             }
         }
 
@@ -720,8 +689,8 @@ private struct State : IEquatable<State>
             private const uint SpinnerCountIncrement = (uint)1 << 2; // bits 2-4
             private const uint SpinnerCountMask = (uint)0x7 << 2;
             private const uint IsWaiterSignaledToWakeMask = (uint)1 << 5; // bit 5
-            private const byte WaiterCountShift = 6;
-            private const uint WaiterCountIncrement = (uint)1 << WaiterCountShift; // bits 6-31
+            private const uint UseTrivialWaitsMask = (uint)1 << 6; // bit 6
+            private const uint WaiterCountIncrement = (uint)1 << 7; // bits 7-31
 
             private uint _state;
 
@@ -800,6 +769,22 @@ private void ClearIsWaiterSignaledToWake()
                 _state -= IsWaiterSignaledToWakeMask;
             }
 
+            // Trivial waits are:
+            // - Not interruptible by Thread.Interrupt
+            // - Don't allow reentrance through APCs or message pumping
+            // - Not forwarded to SynchronizationContext wait overrides
+            public bool UseTrivialWaits => (_state & UseTrivialWaitsMask) != 0;
+
+            public static void InitializeUseTrivialWaits(Lock lockObj, bool useTrivialWaits)
+            {
+                Debug.Assert(lockObj._state == 0);
+
+                if (useTrivialWaits)
+                {
+                    lockObj._state = UseTrivialWaitsMask;
+                }
+            }
+
             public bool HasAnyWaiters => _state >= WaiterCountIncrement;
 
             private bool TryIncrementWaiterCount()