1
0
Fork 0
mirror of https://github.com/VSadov/Satori.git synced 2025-06-08 03:27:04 +09:00

Reapply revert of https://github.com/dotnet/runtime/pull/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 f1297015e9.

* Fix Lock's waiter duration computation

PR https://github.com/dotnet/runtime/pull/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 https://github.com/dotnet/runtime/issues/98021

* Fix wraparound issue

* Fix recording waiter start time

* Use a bit in the _state field instead
This commit is contained in:
Koundinya Veluri 2024-03-06 22:05:58 -08:00 committed by GitHub
parent ed1e0abeb6
commit 77141aea64
Signed by: github
GPG key ID: B5690EEEBB952194
24 changed files with 117 additions and 67 deletions

View file

@ -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)
{

View file

@ -65,7 +65,7 @@ namespace System.Collections.Concurrent
{
protected ConcurrentUnifier()
{
_lock = new Lock();
_lock = new Lock(useTrivialWaits: true);
_container = new Container(this);
}

View file

@ -75,7 +75,7 @@ namespace System.Collections.Concurrent
{
protected ConcurrentUnifierW()
{
_lock = new Lock();
_lock = new Lock(useTrivialWaits: true);
_container = new Container(this);
}

View file

@ -84,7 +84,7 @@ namespace System.Collections.Concurrent
{
protected ConcurrentUnifierWKeyed()
{
_lock = new Lock();
_lock = new Lock(useTrivialWaits: true);
_container = new Container(this);
}

View file

@ -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>

View file

@ -16,7 +16,7 @@ namespace Internal.Runtime
{
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 @@ namespace Internal.Runtime
{
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 @@ namespace Internal.Runtime
Debug.Assert(obj != null);
}
} // end of m_Crst lock
finally
{
m_Crst.Release();
}
IntPtr result = (IntPtr)obj;

View file

@ -275,7 +275,7 @@ namespace System.Runtime.CompilerServices
#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 @@ namespace System.Runtime.CompilerServices
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 @@ namespace System.Runtime.CompilerServices
internal static void Initialize()
{
s_cctorArrays = new Cctor[10][];
s_cctorGlobalLock = new Lock();
s_cctorGlobalLock = new Lock(useTrivialWaits: true);
}
[Conditional("ENABLE_NOISY_CCTOR_LOG")]

View file

@ -44,7 +44,7 @@ namespace System.Runtime.InteropServices
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)

View file

@ -114,6 +114,7 @@ namespace System.Threading
success =
waiter.ev.WaitOneNoCheck(
millisecondsTimeout,
false, // useTrivialWaits
associatedObjectForMonitorWait,
associatedObjectForMonitorWait != null
? NativeRuntimeEventSource.WaitHandleWaitSourceMap.MonitorWait

View file

@ -92,6 +92,18 @@ namespace System.Threading
_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 @@ namespace System.Threading
case StaticsInitializationStage.Complete:
if (_spinCount == SpinCountNotInitialized)
{
if (!IsFullyInitialized)
{
goto case StaticsInitializationStage.Started;
}
_spinCount = s_maxSpinCount;
}
return TryLockResult.Spin;
@ -121,7 +137,7 @@ namespace System.Threading
}
stage = (StaticsInitializationStage)Volatile.Read(ref s_staticsInitializationStage);
if (stage == StaticsInitializationStage.Complete)
if (stage == StaticsInitializationStage.Complete && IsFullyInitialized)
{
goto case StaticsInitializationStage.Complete;
}
@ -166,14 +182,17 @@ namespace System.Threading
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 @@ namespace System.Threading
}
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

View file

@ -65,7 +65,7 @@ namespace System.Threading
/// <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 @@ namespace System.Threading
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>

View file

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

View file

@ -31,7 +31,7 @@ namespace System.Threading
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,

View file

@ -31,15 +31,14 @@ namespace System
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 @@ namespace System
return type;
}
finally
{
AllocationLockHolder.AllocationLock.Release();
}
}
//

View file

@ -24,7 +24,7 @@ namespace Internal.Runtime.TypeLoader
}
// 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)
{

View file

@ -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>();

View file

@ -145,7 +145,7 @@ namespace Internal.Runtime.TypeLoader
}
// 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()
{

View file

@ -18,7 +18,7 @@ namespace Internal.Runtime.TypeLoader
// 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()
{

View file

@ -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;

View file

@ -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);
};

View file

@ -41,6 +41,16 @@ namespace System.Threading
private ushort _waiterStartTimeMs;
private AutoResetEvent? _waitEvent;
#if NATIVEAOT // The method needs to be public in NativeAOT so that other private libraries can access it
public Lock(bool useTrivialWaits)
#else
internal Lock(bool useTrivialWaits)
#endif
: this()
{
State.InitializeUseTrivialWaits(this, useTrivialWaits);
}
/// <summary>
/// Enters the lock. Once the method returns, the calling thread would be the only thread that holds the lock.
/// </summary>
@ -444,9 +454,9 @@ namespace System.Threading
Wait:
bool areContentionEventsEnabled =
NativeRuntimeEventSource.Log?.IsEnabled(
NativeRuntimeEventSource.Log.IsEnabled(
EventLevel.Informational,
NativeRuntimeEventSource.Keywords.ContentionKeyword) ?? false;
NativeRuntimeEventSource.Keywords.ContentionKeyword);
AutoResetEvent waitEvent = _waitEvent ?? CreateWaitEvent(areContentionEventsEnabled);
if (State.TryLockBeforeWait(this))
{
@ -463,7 +473,7 @@ namespace System.Threading
long waitStartTimeTicks = 0;
if (areContentionEventsEnabled)
{
NativeRuntimeEventSource.Log!.ContentionStart(this);
NativeRuntimeEventSource.Log.ContentionStart(this);
waitStartTimeTicks = Stopwatch.GetTimestamp();
}
@ -472,7 +482,7 @@ namespace System.Threading
int remainingTimeoutMs = timeoutMs;
while (true)
{
if (!waitEvent.WaitOne(remainingTimeoutMs))
if (!waitEvent.WaitOneNoCheck(remainingTimeoutMs, new State(this).UseTrivialWaits))
{
break;
}
@ -535,7 +545,7 @@ namespace System.Threading
{
double waitDurationNs =
(Stopwatch.GetTimestamp() - waitStartTimeTicks) * 1_000_000_000.0 / Stopwatch.Frequency;
NativeRuntimeEventSource.Log!.ContentionStop(waitDurationNs);
NativeRuntimeEventSource.Log.ContentionStop(waitDurationNs);
}
return currentThreadId;
@ -574,7 +584,7 @@ namespace System.Threading
ushort waiterStartTimeMs = _waiterStartTimeMs;
return
waiterStartTimeMs != 0 &&
(ushort)Environment.TickCount - waiterStartTimeMs >= MaxDurationMsForPreemptingWaiters;
(ushort)(Environment.TickCount - waiterStartTimeMs) >= MaxDurationMsForPreemptingWaiters;
}
}
@ -670,8 +680,8 @@ namespace System.Threading
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;
@ -750,6 +760,22 @@ namespace System.Threading
_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()

View file

@ -7,8 +7,8 @@ namespace System.Threading
{
public abstract partial class WaitHandle
{
private static int WaitOneCore(IntPtr handle, int millisecondsTimeout) =>
WaitSubsystem.Wait(handle, millisecondsTimeout, true);
private static int WaitOneCore(IntPtr handle, int millisecondsTimeout, bool useTrivialWaits) =>
WaitSubsystem.Wait(handle, millisecondsTimeout, interruptible: !useTrivialWaits);
private static int WaitMultipleIgnoringSyncContextCore(Span<IntPtr> handles, bool waitAll, int millisecondsTimeout) =>
WaitSubsystem.Wait(handles, waitAll, millisecondsTimeout);

View file

@ -14,11 +14,11 @@ namespace System.Threading
{
fixed (IntPtr* pHandles = &MemoryMarshal.GetReference(handles))
{
return WaitForMultipleObjectsIgnoringSyncContext(pHandles, handles.Length, waitAll, millisecondsTimeout);
return WaitForMultipleObjectsIgnoringSyncContext(pHandles, handles.Length, waitAll, millisecondsTimeout, useTrivialWaits: false);
}
}
private static unsafe int WaitForMultipleObjectsIgnoringSyncContext(IntPtr* pHandles, int numHandles, bool waitAll, int millisecondsTimeout)
private static unsafe int WaitForMultipleObjectsIgnoringSyncContext(IntPtr* pHandles, int numHandles, bool waitAll, int millisecondsTimeout, bool useTrivialWaits)
{
Debug.Assert(millisecondsTimeout >= -1);
@ -27,7 +27,8 @@ namespace System.Threading
waitAll = false;
#if NATIVEAOT // TODO: reentrant wait support https://github.com/dotnet/runtime/issues/49518
bool reentrantWait = Thread.ReentrantWaitsEnabled;
// Trivial waits don't allow reentrance
bool reentrantWait = !useTrivialWaits && Thread.ReentrantWaitsEnabled;
if (reentrantWait)
{
@ -92,9 +93,9 @@ namespace System.Threading
return result;
}
internal static unsafe int WaitOneCore(IntPtr handle, int millisecondsTimeout)
internal static unsafe int WaitOneCore(IntPtr handle, int millisecondsTimeout, bool useTrivialWaits)
{
return WaitForMultipleObjectsIgnoringSyncContext(&handle, 1, false, millisecondsTimeout);
return WaitForMultipleObjectsIgnoringSyncContext(&handle, 1, false, millisecondsTimeout, useTrivialWaits);
}
private static int SignalAndWaitCore(IntPtr handleToSignal, IntPtr handleToWaitOn, int millisecondsTimeout)

View file

@ -106,6 +106,7 @@ namespace System.Threading
internal bool WaitOneNoCheck(
int millisecondsTimeout,
bool useTrivialWaits = false,
object? associatedObject = null,
NativeRuntimeEventSource.WaitHandleWaitSourceMap waitSource = NativeRuntimeEventSource.WaitHandleWaitSourceMap.Unknown)
{
@ -122,22 +123,26 @@ namespace System.Threading
waitHandle.DangerousAddRef(ref success);
int waitResult = WaitFailed;
SynchronizationContext? context = SynchronizationContext.Current;
if (context != null && context.IsWaitNotificationRequired())
// Check if the wait should be forwarded to a SynchronizationContext wait override. Trivial waits don't allow
// reentrance or interruption, and are not forwarded.
bool usedSyncContextWait = false;
if (!useTrivialWaits)
{
waitResult = context.Wait(new[] { waitHandle.DangerousGetHandle() }, false, millisecondsTimeout);
SynchronizationContext? context = SynchronizationContext.Current;
if (context != null && context.IsWaitNotificationRequired())
{
usedSyncContextWait = true;
waitResult = context.Wait(new[] { waitHandle.DangerousGetHandle() }, false, millisecondsTimeout);
}
}
else
if (!usedSyncContextWait)
{
#if !CORECLR // CoreCLR sends the wait events from the native side
bool sendWaitEvents =
millisecondsTimeout != 0 &&
#if NATIVEAOT
// A null check is necessary in NativeAOT due to the possibility of reentrance during class
// construction, as this path can be reached through Lock. See
// https://github.com/dotnet/runtime/issues/94728 for a call stack.
NativeRuntimeEventSource.Log != null &&
#endif
!useTrivialWaits &&
NativeRuntimeEventSource.Log.IsEnabled(
EventLevel.Verbose,
NativeRuntimeEventSource.Keywords.WaitHandleKeyword);
@ -149,7 +154,7 @@ namespace System.Threading
waitSource != NativeRuntimeEventSource.WaitHandleWaitSourceMap.MonitorWait;
if (tryNonblockingWaitFirst)
{
waitResult = WaitOneCore(waitHandle.DangerousGetHandle(), millisecondsTimeout: 0);
waitResult = WaitOneCore(waitHandle.DangerousGetHandle(), 0 /* millisecondsTimeout */, useTrivialWaits);
if (waitResult == WaitTimeout)
{
// Do a full wait and send the wait events
@ -171,7 +176,7 @@ namespace System.Threading
if (!tryNonblockingWaitFirst)
#endif
{
waitResult = WaitOneCore(waitHandle.DangerousGetHandle(), millisecondsTimeout);
waitResult = WaitOneCore(waitHandle.DangerousGetHandle(), millisecondsTimeout, useTrivialWaits);
}
#if !CORECLR // CoreCLR sends the wait events from the native side