diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index db9423528f089f..54fe1dffded2f5 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -29,7 +29,6 @@ #endif // FEATURE_COMINTEROP #include "request_common.h" -#include "conditionalweaktable.h" #ifndef USE_DAC_TABLE_RVA extern "C" bool TryGetSymbol(ICorDebugDataTarget* dataTarget, uint64_t baseAddress, const char* symbolName, uint64_t* symbolAddress); @@ -6100,15 +6099,26 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::EnumerateMonitorEventWaitList(VMP if(psb == NULL) return hr; - FieldDesc* pConditionTableField = (&g_CoreLib)->GetField(FIELD__MONITOR__CONDITION_TABLE); - CONDITIONAL_WEAK_TABLE_REF conditionTable = *(DPTR(CONDITIONAL_WEAK_TABLE_REF))PTR_TO_TADDR(pConditionTableField->GetStaticAddressHandle(pConditionTableField->GetBase())); + // The managed Lock (if any) for this object is referenced from the sync block. + // If there is no Lock, there can be no Condition installed on it and therefore no waiters. + OBJECTHANDLE lockHandle = psb->GetLockIfExists(); + if (lockHandle == (OBJECTHANDLE)NULL) + return hr; + OBJECTREF lockObj = ObjectFromHandle(lockHandle); + if (lockObj == NULL) + return hr; - OBJECTREF condition = NULL; - if (!conditionTable->TryGetValue(OBJECTREF(pObj), &condition)) - { + // Lock._waitEventOrCondition holds either an AutoResetEvent or a Condition. Only when a + // Condition has been installed does the Lock have a Monitor wait list to enumerate. + FieldDesc* pWaitEventOrConditionField = (&g_CoreLib)->GetField(FIELD__LOCK__WAIT_EVENT_OR_CONDITION); + OBJECTREF condition = pWaitEventOrConditionField->GetRefValue(lockObj); + if (condition == NULL) + return hr; + + PTR_MethodTable pConditionMT = CoreLibBinder::GetExistingClass(CLASS__CONDITION); + if (condition->GetMethodTable() != pConditionMT) return hr; - } MapSHash waiterToThreadMap; FieldDesc* pConditionWaiterField = (&g_CoreLib)->GetField(FIELD__CONDITION__WAITERS_HEAD); diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index cc515fd5f610e8..9780fbacbdb913 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -626,7 +626,6 @@ DEFINE_CLASS(OLE_AUT_BINDER, System, OleAutBinder) END_ILLINK_FEATURE_SWITCH() DEFINE_CLASS(MONITOR, Threading, Monitor) -DEFINE_FIELD(MONITOR, CONDITION_TABLE, s_conditionTable) DEFINE_METHOD(MONITOR, SYNCHRONIZED_METHOD_ENTER, SynchronizedMethodEnter, SM_Obj_RefBool_RetVoid) DEFINE_METHOD(MONITOR, SYNCHRONIZED_METHOD_EXIT, SynchronizedMethodExit, SM_Obj_RefBool_RetVoid) @@ -634,6 +633,7 @@ DEFINE_CLASS(LOCK, Threading, Lock) DEFINE_FIELD(LOCK, OWNING_THREAD_ID, _owningThreadId) DEFINE_FIELD(LOCK, STATE, _state) DEFINE_FIELD(LOCK, RECURSION_COUNT, _recursionCount) +DEFINE_FIELD(LOCK, WAIT_EVENT_OR_CONDITION,_waitEventOrCondition) DEFINE_METHOD(LOCK, CTOR, .ctor, IM_RetVoid) DEFINE_METHOD(LOCK, INITIALIZE_FOR_MONITOR, InitializeForMonitor, SM_PtrLock_Int_UInt_PtrException_RetVoid) diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 7e9355d26d52a0..ff44ef3c55770b 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1272,7 +1272,7 @@ - + @@ -2998,4 +2998,4 @@ - + \ No newline at end of file diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Condition.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Condition.cs index 302721b59d8097..34e0834290dba5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Condition.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Condition.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -#pragma warning disable 0420 //passing volatile fields by ref - using System.Diagnostics; using System.Diagnostics.Tracing; @@ -14,8 +12,7 @@ internal sealed class Waiter { public Waiter? next; public Waiter? prev; - public AutoResetEvent ev = new AutoResetEvent(false); - public bool signalled; + public readonly AutoResetEvent ev = new AutoResetEvent(false); } [ThreadStatic] @@ -36,7 +33,7 @@ private static Waiter GetWaiterForCurrentThread() waiter = new Waiter(); } - waiter.signalled = false; + Debug.Assert(waiter.next is null && waiter.prev is null); return waiter; } @@ -47,12 +44,18 @@ private static void ReleaseWaiterForCurrentThread(Waiter waiter) } private readonly Lock _lock; + + // When condition is installed in a Lock it takes the same field as waitEvent would. + // If waitEvent is also needed, it is available through here. + internal AutoResetEvent? _waitEvent; + private Waiter? _waitersHead; private Waiter? _waitersTail; internal Lock AssociatedLock => _lock; - private unsafe void AssertIsInList(Waiter waiter) + [Conditional("DEBUG")] + private void AssertIsInList(Waiter waiter) { Debug.Assert(_waitersHead != null && _waitersTail != null); Debug.Assert((_waitersHead == waiter) == (waiter.prev == null)); @@ -64,7 +67,8 @@ private unsafe void AssertIsInList(Waiter waiter) Debug.Fail("Waiter is not in the waiter list"); } - private unsafe void AssertIsNotInList(Waiter waiter) + [Conditional("DEBUG")] + private void AssertIsNotInList(Waiter waiter) { Debug.Assert(waiter.next == null && waiter.prev == null); Debug.Assert((_waitersHead == null) == (_waitersTail == null)); @@ -74,20 +78,32 @@ private unsafe void AssertIsNotInList(Waiter waiter) Debug.Fail("Waiter is in the waiter list, but should not be"); } - private unsafe void AddWaiter(Waiter waiter) + // Returns true if the waiter cannot be possibly in the list. + // (i.e. not reachable via _waitersHead) + internal bool NotInList(Waiter waiter) + { + return _waitersHead != waiter && waiter.prev == null; + } + + private void AddWaiter(Waiter waiter) { Debug.Assert(_lock.IsHeldByCurrentThread); AssertIsNotInList(waiter); - waiter.prev = _waitersTail; - waiter.prev?.next = waiter; - + Waiter? tail = _waitersTail; + waiter.prev = tail; + if (tail is null) + { + _waitersHead = waiter; + } + else + { + tail.next = waiter; + } _waitersTail = waiter; - - _waitersHead ??= waiter; } - private unsafe void RemoveWaiter(Waiter waiter) + private void RemoveWaiter(Waiter waiter) { Debug.Assert(_lock.IsHeldByCurrentThread); AssertIsInList(waiter); @@ -114,7 +130,7 @@ public Condition(Lock @lock) _lock = @lock; } - public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonitorWait) + public bool Wait(int millisecondsTimeout, object associatedObjectForMonitorWait) { ArgumentOutOfRangeException.ThrowIfLessThan(millisecondsTimeout, -1); @@ -128,6 +144,7 @@ public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonit uint recursionCount = _lock.ExitAll(); bool success = false; + bool wasSignaled; try { success = @@ -142,14 +159,16 @@ public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonit _lock.Reenter(recursionCount); Debug.Assert(_lock.IsHeldByCurrentThread); - if (!waiter.signalled) + // If the waiter is still in the list, it was not signaled. + wasSignaled = NotInList(waiter); + if (!wasSignaled) { RemoveWaiter(waiter); } else if (!success) { // - // The wait timed out, but we were signalled before we could reacquire the lock. + // The wait timed out, but we were signaled before we could reacquire the lock. // Since WaitOne timed out, it didn't trigger the auto-reset of the AutoResetEvent. // So, we need to manually reset the event. // @@ -160,19 +179,39 @@ public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonit ReleaseWaiterForCurrentThread(waiter); } - return waiter.signalled; + return wasSignaled; } - public unsafe void SignalAll() + public void SignalAll() { if (!_lock.IsHeldByCurrentThread) throw new SynchronizationLockException(); - while (_waitersHead != null) - SignalOne(); + Waiter? waiter = _waitersHead; + if (waiter is null) + { + return; + } + + // Detach the entire waiter list in one operation, then walk it and signal each waiter. + // Per-waiter prev/next must be cleared BEFORE calling ev.Set() so that the woken thread + // observes the waiter as not in the list (see NotInList) and the cached Waiter is clean. + // Woken threads cannot make progress until the caller releases _lock, so it is safe to + // continue walking the detached list after signaling each waiter. + _waitersHead = null; + _waitersTail = null; + + while (waiter is not null) + { + Waiter? next = waiter.next; + waiter.next = null; + waiter.prev = null; + waiter.ev.Set(); + waiter = next; + } } - public unsafe void SignalOne() + public void SignalOne() { if (!_lock.IsHeldByCurrentThread) throw new SynchronizationLockException(); @@ -181,7 +220,6 @@ public unsafe void SignalOne() if (waiter != null) { RemoveWaiter(waiter); - waiter.signalled = true; waiter.ev.Set(); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs index f050a4c64b9bfa..7ea7244e1f99fa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs @@ -52,7 +52,39 @@ public sealed class Lock private short _spinCount; private ushort _waiterStartTimeMs; - private AutoResetEvent? _waitEvent; + + private object? _waitEventOrCondition; + private AutoResetEvent? WaitEvent + { + get + { + object? weoc = _waitEventOrCondition; + if (weoc is Condition c) + return c._waitEvent; + + return (AutoResetEvent?)weoc; + } + } + + internal Condition GetOrCreateCondition() + { + // The loop terminates as _waitEventOrCondition has limited number of + // state transitions with no cycles. + while (true) + { + object? weoc = _waitEventOrCondition; + if (weoc is Condition c) + return c; + + Condition newCondition = new Condition(this); + newCondition._waitEvent = WaitEvent; + + if (Interlocked.CompareExchange(ref _waitEventOrCondition, newCondition, weoc) == weoc) + { + return newCondition; + } + } + } /// /// Initializes a new instance of the class. @@ -513,7 +545,8 @@ internal int TryEnterSlow(int timeoutMs, int currentThreadId) NativeRuntimeEventSource.Log.IsEnabled( EventLevel.Informational, NativeRuntimeEventSource.Keywords.ContentionKeyword); - AutoResetEvent waitEvent = _waitEvent ?? CreateWaitEvent(areContentionEventsEnabled); + + AutoResetEvent waitEvent = WaitEvent ?? CreateWaitEvent(areContentionEventsEnabled); if (State.TryLockBeforeWait(this)) { // Lock was acquired and a waiter was not registered @@ -652,8 +685,13 @@ private bool ShouldStopPreemptingWaiters private AutoResetEvent CreateWaitEvent(bool areContentionEventsEnabled) { var newWaitEvent = new AutoResetEvent(false); - AutoResetEvent? waitEventBeforeUpdate = Interlocked.CompareExchange(ref _waitEvent, newWaitEvent, null); - if (waitEventBeforeUpdate == null) + object? weocBeforeUpdate = Interlocked.CompareExchange(ref _waitEventOrCondition, newWaitEvent, null); + if (weocBeforeUpdate is Condition c) + { + weocBeforeUpdate = Interlocked.CompareExchange(ref c._waitEvent, newWaitEvent, null); + } + + if (weocBeforeUpdate == null) { // Also check NativeRuntimeEventSource.Log.IsEnabled() to enable trimming if (areContentionEventsEnabled && NativeRuntimeEventSource.Log.IsEnabled()) @@ -665,7 +703,7 @@ private AutoResetEvent CreateWaitEvent(bool areContentionEventsEnabled) } newWaitEvent.Dispose(); - return waitEventBeforeUpdate; + return (AutoResetEvent)weocBeforeUpdate; } [MethodImpl(MethodImplOptions.NoInlining)] @@ -674,8 +712,8 @@ private void SignalWaiterIfNecessary(State state) if (State.TrySetIsWaiterSignaledToWake(this, state)) { // Signal a waiter to wake - Debug.Assert(_waitEvent != null); - bool signaled = _waitEvent.Set(); + Debug.Assert(WaitEvent != null); + bool signaled = WaitEvent.Set(); Debug.Assert(signaled); } } @@ -695,14 +733,14 @@ public bool IsHeldByCurrentThread } internal static long ContentionCount => s_contentionCount; - internal void Dispose() => _waitEvent?.Dispose(); + internal void Dispose() => WaitEvent?.Dispose(); internal nint LockIdForEvents { get { - Debug.Assert(_waitEvent != null); - return _waitEvent.SafeWaitHandle.DangerousGetHandle(); + Debug.Assert(WaitEvent != null); + return WaitEvent.SafeWaitHandle.DangerousGetHandle(); } } @@ -927,6 +965,28 @@ private static short DetermineMinSpinCountForAdaptiveSpin() return (short)-adaptiveSpinPeriod; } + internal bool Wait(int millisecondsTimeout) + { +#pragma warning disable CS9216 // A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in 'lock' statement. + return Wait(millisecondsTimeout, this); +#pragma warning restore CS9216 // A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in 'lock' statement. + } + + internal bool Wait(int millisecondsTimeout, object associatedObject) + { + return GetOrCreateCondition().Wait(millisecondsTimeout, associatedObject); + } + + internal void Pulse() + { + GetOrCreateCondition().SignalOne(); + } + + internal void PulseAll() + { + GetOrCreateCondition().SignalAll(); + } + private struct State : IEquatable { // Layout constants for Lock._state diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs index c07fb07a8328d3..0ececc08ec100b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs @@ -37,18 +37,19 @@ public class ManualResetEventSlim : IDisposable // These are the default spin counts we use on single-proc and MP machines. private const int DEFAULT_SPIN_SP = 1; - private object? m_lock; // A lock used for waiting and pulsing. Lazily initialized via EnsureLockObjectCreated() + // We are not using thin/object lock here since our use pattern makes it nearly certain that + // a thin lock would be inflated even if we use m_lock only once. + private Lock? m_lock; - private volatile ManualResetEvent? m_eventObj; // A true Win32 event used for waiting. + private ManualResetEvent? m_eventObj; // A true Win32 event used for waiting. // -- State -- // // For a packed word a uint would seem better, but Interlocked.* doesn't support them as uint isn't CLS-compliant. - private volatile int m_combinedState; // ie a uint. Used for the state items listed below. + private int m_combinedState; // ie a uint. Used for the state items listed below. // 1-bit for signalled state private const int SignalledState_BitMask = unchecked((int)0x80000000); // 1000 0000 0000 0000 0000 0000 0000 0000 - private const int SignalledState_ShiftCount = 31; // 1-bit for disposed state private const int Dispose_BitMask = unchecked((int)0x40000000); // 0100 0000 0000 0000 0000 0000 0000 0000 @@ -60,7 +61,6 @@ public class ManualResetEventSlim : IDisposable // 19-bits for m_waiters. This allows support of 512K threads waiting which should be ample private const int NumWaitersState_BitMask = unchecked((int)0x0007FFFF); // 0000 0000 0000 0111 1111 1111 1111 1111 - private const int NumWaitersState_ShiftCount = 0; private const int NumWaitersState_MaxValue = (1 << 19) - 1; // 512K-1 // ----------- // @@ -68,7 +68,7 @@ public class ManualResetEventSlim : IDisposable /// Gets the underlying object for this . /// - /// The underlying event object fore this The underlying event object for this . /// /// Accessing this property forces initialization of an underlying event object if one hasn't @@ -97,8 +97,23 @@ public WaitHandle WaitHandle /// true if the event has is set; otherwise, false. public bool IsSet { - get => 0 != ExtractStatePortion(m_combinedState, SignalledState_BitMask); - private set => UpdateStateAtomically(((value) ? 1 : 0) << SignalledState_ShiftCount, SignalledState_BitMask); + get + { + // Volatile to make sure checking IsSet is not optimized away or reordered + // when called as a public API. + return Volatile.Read(ref m_combinedState) < 0; + } + private set + { + if (value) + { + Interlocked.Or(ref m_combinedState, SignalledState_BitMask); + } + else + { + Interlocked.And(ref m_combinedState, ~SignalledState_BitMask); + } + } } /// @@ -116,25 +131,34 @@ private set } } - /// - /// How many threads are waiting. - /// - private int Waiters + private void InterlockedIncrementWaiters() { - get => ExtractStatePortionAndShiftRight(m_combinedState, NumWaitersState_BitMask, NumWaitersState_ShiftCount); - set + int currentState = m_combinedState; + while (true) { - // setting to <0 would indicate an internal flaw, hence Assert is appropriate. - Debug.Assert(value >= 0, "NumWaiters should never be less than zero. This indicates an internal error."); - // it is possible for the max number of waiters to be exceeded via user-code, hence we use a real exception here. - if (value >= NumWaitersState_MaxValue) + if ((currentState & NumWaitersState_BitMask) >= NumWaitersState_MaxValue) throw new InvalidOperationException(SR.Format(SR.ManualResetEventSlim_ctor_TooManyWaiters, NumWaitersState_MaxValue)); - UpdateStateAtomically(value << NumWaitersState_ShiftCount, NumWaitersState_BitMask); + // The waiters count has room for one more, so adding 1 to the full state + // cannot carry out of NumWaitersState_BitMask. + int oldState = Interlocked.CompareExchange(ref m_combinedState, currentState + 1, currentState); + if (oldState == currentState) + return; + + // Highly unlikely contention, since we increment/decrement while holding a lock, just try again. + currentState = oldState; } } + private void InterlockedDecrementWaiters() + { + // setting to <0 would indicate an internal flaw, hence Assert is appropriate. + Debug.Assert((m_combinedState & NumWaitersState_BitMask) != 0, "NumWaiters should never be less than zero. This indicates an internal error."); + + Interlocked.Decrement(ref m_combinedState); + } + //----------------------------------------------------------------------------------- // Constructs a new event, optionally specifying the initial state and spin count. // The defaults are that the event is unsignaled and some reasonable default spin. @@ -190,7 +214,8 @@ public ManualResetEventSlim(bool initialState, int spinCount) #pragma warning disable IDE0060 // Remove unused parameter spinCount, on single-threaded systems, the spin count is not used. private void Initialize(bool initialState, int spinCount) { - m_combinedState = initialState ? (1 << SignalledState_ShiftCount) : 0; + m_combinedState = initialState ? SignalledState_BitMask : 0; + // the spinCount argument has been validated by the ctors. // but we now sanity check our predefined constants. Debug.Assert(DEFAULT_SPIN_SP >= 0, "Internal error - DEFAULT_SPIN_SP is outside the legal range."); @@ -208,12 +233,12 @@ private void EnsureLockObjectCreated() { if (m_lock is null) { - Interlocked.CompareExchange(ref m_lock, new object(), null); // failure is benign. Someone else set the value. + Interlocked.CompareExchange(ref m_lock, new Lock(), null); // failure is benign. Someone else set the value. } } /// - /// This method lazily initializes the event object. It uses CAS to guarantee that + /// This method lazily initializes the public wait object. It uses CAS to guarantee that /// many threads racing to call this at once don't result in more than one event /// being stored and used. The event will be signaled or unsignaled depending on /// the state of the thin-event itself, with synchronization taken into account. @@ -235,7 +260,7 @@ private void LazyInitializeEvent() // Now that the event is published, verify that the state hasn't changed since // we snapped the preInitializeState. Another thread could have done that // between our initial observation above and here. The barrier incurred from - // the CAS above (in addition to m_state being volatile) prevents this read + // the CAS above prevents this read // from moving earlier and being collapsed with our original one. bool currentIsSet = IsSet; if (currentIsSet != preInitializeIsSet) @@ -272,18 +297,17 @@ public void Set() /// The object has been canceled. private void Set(bool duringCancellation) { - // We need to ensure that IsSet=true does not get reordered past the read of m_eventObj - // This would be a legal movement according to the .NET memory model. - // The code is safe as IsSet involves an Interlocked.CompareExchange which provides a full memory barrier. - IsSet = true; + // We need to ensure that the state change happens before we read the m_eventObj. + // Interlocked guarantees that. + long origState = Interlocked.Or(ref m_combinedState, SignalledState_BitMask); - // If there are waiting threads, we need to pulse them. - if (Waiters > 0) + // If there were waiting threads when we flipped the state, we need to pulse them. + if ((origState & NumWaitersState_BitMask) != 0) { Debug.Assert(m_lock != null); // if waiters>0, then m_lock has already been created. - lock (m_lock) + using (m_lock.EnterScope()) { - Monitor.PulseAll(m_lock); + m_lock.PulseAll(); } } @@ -502,7 +526,6 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) // We spin briefly before falling back to allocating and/or waiting on a true event. long startTime = 0; - bool bNeedTimeoutAdjustment = false; int realMillisecondsTimeout = millisecondsTimeout; // this will be adjusted if necessary. if (millisecondsTimeout != Timeout.Infinite) @@ -514,7 +537,6 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) // decide to block in the kernel below. startTime = Environment.TickCount64; - bNeedTimeoutAdjustment = true; } // Spin @@ -540,36 +562,35 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) // We must register and unregister the token outside of the lock, to avoid deadlocks. using (cancellationToken.UnsafeRegister(s_cancellationTokenCallback, this)) { - lock (m_lock) + // Loop to cope with spurious wakeups from other waits being canceled + while (!IsSet) { - // Loop to cope with spurious wakeups from other waits being canceled - while (!IsSet) - { - // If our token was canceled, we must throw and exit. - cancellationToken.ThrowIfCancellationRequested(); + // If our token was canceled, we must throw and exit. + cancellationToken.ThrowIfCancellationRequested(); - // update timeout (delays in wait commencement are due to spinning and/or spurious wakeups from other waits being canceled) - if (bNeedTimeoutAdjustment) - { - // TimeoutHelper.UpdateTimeOut returns a long but the value is capped as millisecondsTimeout is an int. - realMillisecondsTimeout = (int)TimeoutHelper.UpdateTimeOut(startTime, millisecondsTimeout); - if (realMillisecondsTimeout <= 0) - return false; - } + // update timeout (delays in wait commencement are due to spinning and/or spurious wakeups from other waits being canceled) + if (startTime != 0) + { + // TimeoutHelper.UpdateTimeOut returns a long but the value is capped as millisecondsTimeout is an int. + realMillisecondsTimeout = (int)TimeoutHelper.UpdateTimeOut(startTime, millisecondsTimeout); + if (realMillisecondsTimeout <= 0) + return false; + } + using (m_lock.EnterScope()) + { // There is a race condition that Set will fail to see that there are waiters as Set does not take the lock, // so after updating waiters, we must check IsSet again. // Also, we must ensure there cannot be any reordering of the assignment to Waiters and the - // read from IsSet. This is guaranteed as Waiters{set;} involves an Interlocked.CompareExchange - // operation which provides a full memory barrier. + // read from IsSet. This is guaranteed as InterlockedIncrementWaiters is a full memory barrier. // If we see IsSet=false, then we are guaranteed that Set() will see that we are // waiting and will pulse the monitor correctly. - Waiters++; + InterlockedIncrementWaiters(); if (IsSet) // This check must occur after updating Waiters. { - Waiters--; // revert the increment. + InterlockedDecrementWaiters(); // revert the increment. return true; } @@ -577,13 +598,13 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) try { // ** the actual wait ** - if (!Monitor.Wait(m_lock, realMillisecondsTimeout)) + if (!m_lock.Wait(realMillisecondsTimeout)) return false; // return immediately if the timeout has expired. } finally { // Clean up: we're done waiting. - Waiters--; + InterlockedDecrementWaiters(); } // Now just loop back around, and the right thing will happen. Either: // 1. We had a spurious wake-up due to some other wait being canceled via a different cancellationToken (rewait) @@ -652,41 +673,9 @@ private static void CancellationTokenCallback(object? obj) Debug.Assert(obj is ManualResetEventSlim, "Expected a ManualResetEventSlim"); ManualResetEventSlim mre = (ManualResetEventSlim)obj; Debug.Assert(mre.m_lock != null); // the lock should have been created before this callback is registered for use. - lock (mre.m_lock) + using (mre.m_lock.EnterScope()) { - Monitor.PulseAll(mre.m_lock); // awaken all waiters - } - } - - /// - /// Private helper method for updating parts of a bit-string state value. - /// Mainly called from the IsSet and Waiters properties setters - /// - /// - /// Note: the parameter types must be int as CompareExchange cannot take a Uint - /// - /// The new value - /// The mask used to set the bits - private void UpdateStateAtomically(int newBits, int updateBitsMask) - { - SpinWait sw = default; - - Debug.Assert((newBits | updateBitsMask) == updateBitsMask, "newBits do not fall within the updateBitsMask."); - - while (true) - { - int oldState = m_combinedState; // cache the old value for testing in CAS - - // Procedure:(1) zero the updateBits. eg oldState = [11111111] flag= [00111000] newState = [11000111] - // then (2) map in the newBits. eg [11000111] newBits=00101000, newState=[11101111] - int newState = (oldState & ~updateBitsMask) | newBits; - - if (Interlocked.CompareExchange(ref m_combinedState, newState, oldState) == oldState) - { - return; - } - - sw.SpinOnce(sleep1Threshold: -1); + mre.m_lock.PulseAll(); // awaken all waiters } } @@ -706,20 +695,5 @@ private static int ExtractStatePortionAndShiftRight(int state, int mask, int rig // then convert back to int. return unchecked((int)(((uint)(state & mask)) >> rightBitShiftCount)); } - - /// - /// Performs a Mask operation, but does not perform the shift. - /// This is acceptable for boolean values for which the shift is unnecessary - /// eg (val & Mask) != 0 is an appropriate way to extract a boolean rather than using - /// ((val & Mask) >> shiftAmount) == 1 - /// - /// ?? is there a common place to put this rather than being private to MRES? - /// - /// - /// - private static int ExtractStatePortion(int state, int mask) - { - return state & mask; - } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Monitor.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Monitor.cs index 0eaf0a79c14e9c..3d590d94ba1f26 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Monitor.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Monitor.cs @@ -78,41 +78,29 @@ private static void ThrowLockTakenException() throw new ArgumentException(SR.Argument_MustBeFalse, "lockTaken"); } - #region Object->Condition mapping - - private static readonly ConditionalWeakTable s_conditionTable = []; - private static readonly Func s_createCondition = (o) => new Condition(GetLockObject(o)); - - private static Condition GetCondition(object obj) - { - Debug.Assert( - obj is not Condition, - "Do not use Monitor.Pulse or Wait on a Condition instance; use the methods on Condition instead."); - return s_conditionTable.GetOrAdd(obj, s_createCondition); - } - #endregion - #region Public Wait/Pulse methods [UnsupportedOSPlatform("browser")] public static bool Wait(object obj, int millisecondsTimeout) { + ArgumentNullException.ThrowIfNull(obj); RuntimeFeature.ThrowIfMultithreadingIsNotSupported(); - return GetCondition(obj).Wait(millisecondsTimeout, obj); + + return GetLockObject(obj).Wait(millisecondsTimeout, obj); } public static void Pulse(object obj) { ArgumentNullException.ThrowIfNull(obj); - GetCondition(obj).SignalOne(); + GetLockObject(obj).Pulse(); } public static void PulseAll(object obj) { ArgumentNullException.ThrowIfNull(obj); - GetCondition(obj).SignalAll(); + GetLockObject(obj).PulseAll(); } #endregion diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadBlockingInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadBlockingInfo.cs index 15155433ed50d7..b371ef5094fbe9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadBlockingInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadBlockingInfo.cs @@ -86,11 +86,9 @@ public int LockOwnerManagedThreadId // the getter may be used by debuggers case ObjectKind.Lock: return ((Lock)Unsafe.AsRef(_objectPtr)).OwningManagedThreadId; -#if !MONO case ObjectKind.Condition: Debug.Assert(_objectKind == ObjectKind.Condition); return ((Condition)Unsafe.AsRef(_objectPtr)).AssociatedLock.OwningManagedThreadId; -#endif default: throw new UnreachableException(); @@ -107,9 +105,7 @@ public ref struct Scope public Scope(Lock lockObj, int timeoutMs) : this(lockObj, ObjectKind.Lock, timeoutMs) { } #pragma warning restore CS9216 -#if !MONO public Scope(Condition condition, int timeoutMs) : this(condition, ObjectKind.Condition, timeoutMs) { } -#endif private Scope(object obj, ObjectKind objectKind, int timeoutMs) {