From 9028f002692761bd893e9b6ece15de34d4cc0b64 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 30 Jan 2023 15:59:17 -0500 Subject: [PATCH 01/43] [mono] Implement synch block fast paths in managed Investigate one incremental alternative from https://github.com/dotnet/runtime/issues/48058 Rather than switching to NativeAOT's Monitor implementation in one step, first get access to Mono's synchronization block from managed, and implement the fast paths for hash code and monitor code in C#. --- .../System.Private.CoreLib.csproj | 1 + .../CompilerServices/RuntimeHelpers.Mono.cs | 4 + .../src/System/Threading/Monitor.Mono.cs | 1 + .../src/System/Threading/ObjectHeader.Mono.cs | 250 ++++++++++++++++++ .../src/System/Threading/Thread.Mono.cs | 2 + 5 files changed, 258 insertions(+) create mode 100644 src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs diff --git a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj index db2a7d21781a70..c40c41e3aa09a6 100644 --- a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -266,6 +266,7 @@ + diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs index 0f590d3492d056..3e3ca4db557cc4 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs @@ -41,6 +41,8 @@ public static int OffsetToStringData public static int GetHashCode(object? o) { + if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) + return hash; return InternalGetHashCode(o); } @@ -57,6 +59,8 @@ public static int GetHashCode(object? o) /// internal static int TryGetHashCode(object? o) { + if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) + return hash; return InternalTryGetHashCode(o); } diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index e004a373ee7719..2b63ef57d7b2c6 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -43,6 +43,7 @@ public static void TryEnter(object obj, ref bool lockTaken) public static bool TryEnter(object obj, int millisecondsTimeout) { bool lockTaken = false; + TryEnter(obj, millisecondsTimeout, ref lockTaken); return lockTaken; } diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs new file mode 100644 index 00000000000000..dd050edcd14941 --- /dev/null +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -0,0 +1,250 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +namespace System.Threading; + +/// +/// Manipulates the object header located in the first few words of each object in the managed heap. +/// +internal static class ObjectHeader +{ + [StructLayout(LayoutKind.Sequential)] + private unsafe struct Header + { +#region Keep in sync with src/native/public/mono/metadata/details/object-types.h + public IntPtr vtable; + public IntPtr synchronization; // really a LockWord +#endregion // keep in sync with src/native/public/mono/metadata/details/object-types.h + } + + [StructLayout(LayoutKind.Sequential)] + private unsafe struct MonoThreadsSync + { +#region Keep in sync with monitor.h + // FIXME: volatile? + public uint status; + public uint nest; + public int hash_code; + // Note: more fields after here +#endregion // keep in sync with monitor.h + } + + // + // A union that contains either an uninflated lockword, or a pointer to a synchronization struct + // + [StructLayout(LayoutKind.Explicit)] + private struct LockWord + { +#region Keep in sync with monitor.h + + [FieldOffset(0)] + private IntPtr _lock_word; + [FieldOffset(0)] + private unsafe MonoThreadsSync* _sync; + + private const int StatusBits = 2; + + private const int NestBits = 8; + + private const IntPtr StatusMask = (1 << StatusBits) - 1; + + private const IntPtr NestMask = ((1 << NestBits) - 1) << StatusBits; + + private const int HashShift = StatusBits; + + private const int NestShift = StatusBits; + + private const int OwnerShift = StatusBits + NestBits; + + [Flags] + private enum Status + { + Flat = 0, + HasHash = 1, + Inflated = 2, + } +#endregion // Keep in sync with monitor.h + + public bool IsInflated => (_lock_word & (IntPtr)Status.Inflated) != 0; + + public unsafe MonoThreadsSync* GetInflatedLock() + { + LockWord lw; + lw._sync = default; + lw._lock_word = _lock_word & ~StatusMask; + return lw._sync; + } + + public bool HasHash => (_lock_word & (IntPtr)Status.HasHash) != 0; + + public bool IsFree => _lock_word == IntPtr.Zero; + + public bool IsFlat => (_lock_word & StatusMask) == (IntPtr)Status.Flat; + + public bool IsNested => (_lock_word & NestMask) == NestMask; + + public int FlatHash => (int)(_lock_word >> HashShift); + + public int FlatNest + { + get + { + if (IsFree) + return 0; + /* Inword nest count starts from 0 */ + return 1 + (int)((_lock_word & NestMask) >> NestShift); + } + } + + public bool IsNestMax => (_lock_word & NestMask) == NestMask; + + public LockWord IncrementNest() + { + LockWord res; + unsafe { res._sync = default; } + res._lock_word = _lock_word + (1 << NestShift); + return res; + } + + public LockWord DecrementNest() + { + LockWord res; + unsafe { res._sync = default; } + res._lock_word = _lock_word - (1 << NestShift); + return res; + } + + public int GetOwner() => (int)(_lock_word >> OwnerShift); + + public static LockWord NewThinHash(int hash) + { + LockWord res; + unsafe { res._sync = default; } + res._lock_word = (((IntPtr)(uint)hash) << HashShift) | (IntPtr)Status.HasHash; + return res; + } + + public static unsafe LockWord NewInflated(MonoThreadsSync* sync) + { + LockWord res; + res._lock_word = default; + res._sync = sync; + res._lock_word |= (IntPtr)Status.Inflated; + return res; + } + + public static LockWord NewFlat(int owner) + { + LockWord res; + unsafe { res._sync = default; } + res._lock_word = ((IntPtr)(uint)owner) << OwnerShift; + return res; + } + + public static LockWord FromObjectHeader(ref Header header) + { + LockWord lw; + unsafe { lw._sync = default; } + lw._lock_word = header.synchronization; + return lw; + } + + public IntPtr AsIntPtr => _lock_word; + } + + private static LockWord GetLockWord(ref object obj) + { + ref Header h = ref Unsafe.As(ref obj); + LockWord lw = LockWord.FromObjectHeader(ref h); + GC.KeepAlive(obj); + return lw; + } + + private static IntPtr LockWordCompareExchange (ref object obj, LockWord nlw, LockWord expected) + { + ref Header h = ref Unsafe.As(ref obj); + return Interlocked.CompareExchange (ref h.synchronization, nlw.AsIntPtr, expected.AsIntPtr); + } + + /// + /// Tries to get the hash code from the object if it is + /// already known and return true. Otherwise returns false. + /// + public static bool TryGetHashCode(object? o, out int hash) + { + hash = 0; + if (o == null) + return true; + + LockWord lw = GetLockWord (ref o); + if (lw.HasHash) { + if (lw.IsInflated) { + unsafe { + hash = lw.GetInflatedLock()->hash_code; + return true; + } + } else { + hash = lw.FlatHash; + return true; + } + } + return false; + } + +#if false // WIP + public static bool TryEnterFast(object? o, ref bool lockTaken) + { + if (lockTaken || o == null) + return false; + + LockWord lw = GetLockWord (ref o); + if (lw.IsFree) + { + int owner = Thread.CurrentThread.GetSmallId(); + LockWord nlw = LockWord.NewFlat(owner); + if (LockWordCompareExchnage (ref o, nlw, lw) == lw.AsIntPtr) + { + lockTaken = true; + return true; + } else { + // someone acquired it in the meantime or put in a hash + Inflate(o); + return TryEnterInflated(o, ref lockTaken); + } + } + else if (lw.IsInflated) + return TryEnterInflated(o, ref lockTaken); + else if (lw.IsFlat) + { + int owner = Thread.CurrentThread.GetSmallId(); + if (lw.GetOwner() == owner) + { + if (lw.IsMaxNest) + { + InflateOwned(o); + return TryEnterInflated(o, ref lockTaken); + } + else + { + LockWord nlw = lw.IncrementNest(); + IntPtr prev = LockWordCompareExchange(ref o, nlw, lw); + if (prev != lw.AsIntPtr) + { + // Someone else inflated it in the meantime + return TryEnterInflated(o, ref lockTaken); + } + lockTaken = true; + return true; + } + } + } + Debug.Assert (lw.HasHash); + Inflate(o); + return TryEnterInflated(o, ref lockTaken); + } +#endif + +} diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs index 528ad1ab12be20..db046e34f5232d 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs @@ -350,5 +350,7 @@ private static void SpinWait_nop() [MethodImplAttribute(MethodImplOptions.InternalCall)] private static extern void SetPriority(Thread thread, int priority); + + private int GetSmallId() => small_id; } } From 10a6c3df8119f6fae894efa7ac87ee9ab3a4b48d Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 30 Jan 2023 16:53:46 -0500 Subject: [PATCH 02/43] Implement TryEnterFast and TryEnterInflatedFast This should help the interpreter particularly, since it doesn't have an intrinsic path here --- .../src/System/Threading/Monitor.Mono.cs | 7 + .../src/System/Threading/ObjectHeader.Mono.cs | 126 ++++++++++++++++-- .../src/System/Threading/Thread.Mono.cs | 2 +- 3 files changed, 125 insertions(+), 10 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index 2b63ef57d7b2c6..c27d94dec4f980 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -19,6 +19,10 @@ public static void Enter(object obj, ref bool lockTaken) if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); + // fast path + if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) + return; + ReliableEnterTimeout(obj, (int)Timeout.Infinite, ref lockTaken); } @@ -37,6 +41,9 @@ public static void TryEnter(object obj, ref bool lockTaken) if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); + if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) + return; + ReliableEnterTimeout(obj, 0, ref lockTaken); } diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index dd050edcd14941..41fc1d4866e7b3 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -32,6 +33,26 @@ private unsafe struct MonoThreadsSync #endregion // keep in sync with monitor.h } + // + // Manipulate the MonoThreadSync:status field + // + private static class MonitorStatus + { +#region Keep in sync with monitor.h + private const uint OwnerMask = 0x0000ffffu; + private const uint EntryCountMask = 0xffff0000u; + //private const uint EntryCountWaiters = 0x80000000u; + //private const uint EntryCountZero = 0x7fff0000u; + //private const int EntryCountShift = 16; +#endregion // keep in sync with monitor.h + + public static int GetOwner(uint status) => (int)(status & OwnerMask); + public static uint SetOwner (uint status, int owner) + { + return (status & EntryCountMask) | (uint)owner; + } + } + // // A union that contains either an uninflated lockword, or a pointer to a synchronization struct // @@ -153,6 +174,11 @@ public static LockWord FromObjectHeader(ref Header header) } public IntPtr AsIntPtr => _lock_word; + + internal void SetFromIntPtr (IntPtr new_lw) + { + _lock_word = new_lw; + } } private static LockWord GetLockWord(ref object obj) @@ -194,7 +220,80 @@ public static bool TryGetHashCode(object? o, out int hash) return false; } -#if false // WIP +#if false // need to implement alloc_mon/discard_mon + private static void Inflate(object o) + { + unsafe { + MonoThreadsSync *mon = alloc_mon(); + LockWord nlw = LockWord.NewInflated (mon); + LockWord old_lw = GetLockWord (ref o); + while (true) + { + if (old_lw.IsInflated) + break; + else if (old_lw.HasHash) + { + mon->hash_code = old_lw.FlatHash; + mon->status = SyncSetOwner (mon->status, 0); + nlw = nlw.SetHasHash(); + + } + else if (old_lw.IsFree) + { + mon->status = SyncSetOwner (mon->status, old_lw.GetOwner()); + mon->nest = old_lw.FlatNest; + } + Interlocked.MemoryBarrier(); + IntPtr prev = LockWordCompareExchange(ref o, nlw, old_lw); + if (prev == old_lw.AsIntPtr) + return; // success + old_lw.SetFromIntPtr(prev); // go around one more time + } + // someone else inflated it first + discard_mon (mon); + } + } +#endif + + private static bool TryEnterInflatedFast(object o, ref bool lockTaken) + { + LockWord lw = GetLockWord (ref o); + int small_id = Thread.CurrentThread.GetSmallId(); + unsafe { + MonoThreadsSync *mon = lw.GetInflatedLock(); + while (true) + { + uint old_status = mon->status; + if (MonitorStatus.GetOwner(old_status) == 0) + { + uint new_status = MonitorStatus.SetOwner(old_status, small_id); + uint prev_status = Interlocked.CompareExchange (ref mon->status, new_status, old_status); + if (prev_status == old_status) + { + lockTaken = true; + return true; + } + // someone else changed the status, go around the loop again + } + if (MonitorStatus.GetOwner(old_status) == small_id) + { + // we own it + mon->nest++; + lockTaken = true; + return true; + } + else + { + // someone else owns it, fall back to slow path + return false; + } + } + } + } + + // returns false if we should fall back to the slow path + // returns true if we tried to enter + // sets lockTaken to true if the lock was taken public static bool TryEnterFast(object? o, ref bool lockTaken) { if (lockTaken || o == null) @@ -205,27 +304,34 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) { int owner = Thread.CurrentThread.GetSmallId(); LockWord nlw = LockWord.NewFlat(owner); - if (LockWordCompareExchnage (ref o, nlw, lw) == lw.AsIntPtr) + if (LockWordCompareExchange (ref o, nlw, lw) == lw.AsIntPtr) { lockTaken = true; return true; } else { + return false; +#if false // someone acquired it in the meantime or put in a hash Inflate(o); - return TryEnterInflated(o, ref lockTaken); + return TryEnterInflatedFast(o, ref lockTaken); +#endif } } else if (lw.IsInflated) - return TryEnterInflated(o, ref lockTaken); + { + return TryEnterInflatedFast(o, ref lockTaken); + } else if (lw.IsFlat) { + return false; +#if false int owner = Thread.CurrentThread.GetSmallId(); if (lw.GetOwner() == owner) { if (lw.IsMaxNest) { InflateOwned(o); - return TryEnterInflated(o, ref lockTaken); + return TryEnterInflatedFast(o, ref lockTaken); } else { @@ -234,17 +340,19 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) if (prev != lw.AsIntPtr) { // Someone else inflated it in the meantime - return TryEnterInflated(o, ref lockTaken); + return TryEnterInflatedFast(o, ref lockTaken); } lockTaken = true; return true; } } +#endif } Debug.Assert (lw.HasHash); + return false; +#if false Inflate(o); - return TryEnterInflated(o, ref lockTaken); - } + return TryEnterInflatedFast(o, ref lockTaken); #endif - + } } diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs index db046e34f5232d..31bc824008607c 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs @@ -351,6 +351,6 @@ private static void SpinWait_nop() [MethodImplAttribute(MethodImplOptions.InternalCall)] private static extern void SetPriority(Thread thread, int priority); - private int GetSmallId() => small_id; + internal int GetSmallId() => small_id; } } From 4d622817644b5021ab504d52282c485950ef657e Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 31 Jan 2023 00:34:24 -0500 Subject: [PATCH 03/43] wip --- .../src/System/Threading/ObjectHeader.Mono.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 41fc1d4866e7b3..deb7d6845cc9f4 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -274,6 +274,7 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) return true; } // someone else changed the status, go around the loop again + continue; } if (MonitorStatus.GetOwner(old_status) == small_id) { @@ -299,6 +300,8 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) if (lockTaken || o == null) return false; + lockTaken = false; + LockWord lw = GetLockWord (ref o); if (lw.IsFree) { @@ -346,6 +349,8 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) return true; } } + Inflate(o); + return TryEnterInflatedFast(o, refLockTaken); #endif } Debug.Assert (lw.HasHash); From 9ae85c080ba11fe8151fd4d76c9e6fcb4e3818e0 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 31 Jan 2023 12:53:45 -0500 Subject: [PATCH 04/43] Add replacements for test_owner and test_synchronized icalls ObjectHeader.IsEntered and ObjectHeader.HasOwner, respectively --- .../src/System/Threading/Monitor.Mono.cs | 23 ++-- .../src/System/Threading/ObjectHeader.Mono.cs | 110 +++++++++++------- src/mono/mono/metadata/icall-def.h | 2 - src/mono/mono/metadata/monitor.c | 40 ------- 4 files changed, 74 insertions(+), 101 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index c27d94dec4f980..4148883307b3dc 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -59,13 +59,17 @@ public static void TryEnter(object obj, int millisecondsTimeout, ref bool lockTa { if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); + + if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) + return; + ReliableEnterTimeout(obj, millisecondsTimeout, ref lockTaken); } public static bool IsEntered(object obj) { ArgumentNullException.ThrowIfNull(obj); - return IsEnteredNative(obj); + return ObjectHeader.IsEntered(obj); } [UnsupportedOSPlatform("browser")] @@ -87,15 +91,12 @@ public static void PulseAll(object obj) ObjPulseAll(obj); } - [MethodImplAttribute(MethodImplOptions.InternalCall)] - private static extern bool Monitor_test_synchronised(object obj); - [MethodImplAttribute(MethodImplOptions.InternalCall)] private static extern void Monitor_pulse(object obj); private static void ObjPulse(object obj) { - if (!Monitor_test_synchronised(obj)) + if (!ObjectHeader.HasOwner(obj)) throw new SynchronizationLockException(); Monitor_pulse(obj); @@ -106,7 +107,7 @@ private static void ObjPulse(object obj) private static void ObjPulseAll(object obj) { - if (!Monitor_test_synchronised(obj)) + if (!ObjectHeader.HasOwner(obj)) throw new SynchronizationLockException(); Monitor_pulse_all(obj); @@ -119,7 +120,7 @@ private static bool ObjWait(int millisecondsTimeout, object obj) { if (millisecondsTimeout < 0 && millisecondsTimeout != (int)Timeout.Infinite) throw new ArgumentOutOfRangeException(nameof(millisecondsTimeout)); - if (!Monitor_test_synchronised(obj)) + if (!ObjectHeader.HasOwner(obj)) throw new SynchronizationLockException(); return Monitor_wait(obj, millisecondsTimeout, true); @@ -138,14 +139,6 @@ private static void ReliableEnterTimeout(object obj, int timeout, ref bool lockT try_enter_with_atomic_var(obj, timeout, true, ref lockTaken); } - [MethodImplAttribute(MethodImplOptions.InternalCall)] - private static extern bool Monitor_test_owner(object obj); - - private static bool IsEnteredNative(object obj) - { - return Monitor_test_owner(obj); - } - public static extern long LockContentionCount { [MethodImplAttribute(MethodImplOptions.InternalCall)] diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index deb7d6845cc9f4..f9c665ef56981b 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -63,8 +63,6 @@ private struct LockWord [FieldOffset(0)] private IntPtr _lock_word; - [FieldOffset(0)] - private unsafe MonoThreadsSync* _sync; private const int StatusBits = 2; @@ -91,12 +89,13 @@ private enum Status public bool IsInflated => (_lock_word & (IntPtr)Status.Inflated) != 0; - public unsafe MonoThreadsSync* GetInflatedLock() + public ref MonoThreadsSync GetInflatedLock() { - LockWord lw; - lw._sync = default; - lw._lock_word = _lock_word & ~StatusMask; - return lw._sync; + unsafe + { + IntPtr ptr = _lock_word & ~StatusMask; + return ref Unsafe.AsRef((void*)ptr); + } } public bool HasHash => (_lock_word & (IntPtr)Status.HasHash) != 0; @@ -125,7 +124,6 @@ public int FlatNest public LockWord IncrementNest() { LockWord res; - unsafe { res._sync = default; } res._lock_word = _lock_word + (1 << NestShift); return res; } @@ -133,7 +131,6 @@ public LockWord IncrementNest() public LockWord DecrementNest() { LockWord res; - unsafe { res._sync = default; } res._lock_word = _lock_word - (1 << NestShift); return res; } @@ -143,24 +140,22 @@ public LockWord DecrementNest() public static LockWord NewThinHash(int hash) { LockWord res; - unsafe { res._sync = default; } res._lock_word = (((IntPtr)(uint)hash) << HashShift) | (IntPtr)Status.HasHash; return res; } public static unsafe LockWord NewInflated(MonoThreadsSync* sync) { + IntPtr ptr = (IntPtr)(void*)sync; + ptr |= (IntPtr)Status.Inflated; LockWord res; - res._lock_word = default; - res._sync = sync; - res._lock_word |= (IntPtr)Status.Inflated; + res._lock_word = ptr; return res; } public static LockWord NewFlat(int owner) { LockWord res; - unsafe { res._sync = default; } res._lock_word = ((IntPtr)(uint)owner) << OwnerShift; return res; } @@ -168,7 +163,6 @@ public static LockWord NewFlat(int owner) public static LockWord FromObjectHeader(ref Header header) { LockWord lw; - unsafe { lw._sync = default; } lw._lock_word = header.synchronization; return lw; } @@ -208,10 +202,8 @@ public static bool TryGetHashCode(object? o, out int hash) LockWord lw = GetLockWord (ref o); if (lw.HasHash) { if (lw.IsInflated) { - unsafe { - hash = lw.GetInflatedLock()->hash_code; - return true; - } + hash = lw.GetInflatedLock().hash_code; + return true; } else { hash = lw.FlatHash; return true; @@ -223,7 +215,8 @@ public static bool TryGetHashCode(object? o, out int hash) #if false // need to implement alloc_mon/discard_mon private static void Inflate(object o) { - unsafe { + unsafe + { MonoThreadsSync *mon = alloc_mon(); LockWord nlw = LockWord.NewInflated (mon); LockWord old_lw = GetLockWord (ref o); @@ -259,35 +252,33 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) { LockWord lw = GetLockWord (ref o); int small_id = Thread.CurrentThread.GetSmallId(); - unsafe { - MonoThreadsSync *mon = lw.GetInflatedLock(); - while (true) + ref MonoThreadsSync mon = ref lw.GetInflatedLock(); + while (true) + { + uint old_status = mon.status; + if (MonitorStatus.GetOwner(old_status) == 0) { - uint old_status = mon->status; - if (MonitorStatus.GetOwner(old_status) == 0) - { - uint new_status = MonitorStatus.SetOwner(old_status, small_id); - uint prev_status = Interlocked.CompareExchange (ref mon->status, new_status, old_status); - if (prev_status == old_status) - { - lockTaken = true; - return true; - } - // someone else changed the status, go around the loop again - continue; - } - if (MonitorStatus.GetOwner(old_status) == small_id) + uint new_status = MonitorStatus.SetOwner(old_status, small_id); + uint prev_status = Interlocked.CompareExchange (ref mon.status, new_status, old_status); + if (prev_status == old_status) { - // we own it - mon->nest++; lockTaken = true; return true; } - else - { - // someone else owns it, fall back to slow path - return false; - } + // someone else changed the status, go around the loop again + continue; + } + if (MonitorStatus.GetOwner(old_status) == small_id) + { + // we own it + mon.nest++; + lockTaken = true; + return true; + } + else + { + // someone else owns it, fall back to slow path + return false; } } } @@ -360,4 +351,35 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) return TryEnterInflatedFast(o, ref lockTaken); #endif } + + // true if obj is owned by the current thread + public static bool IsEntered(object obj) + { + LockWord lw = GetLockWord(ref obj); + + if (lw.IsFlat) + { + return lw.GetOwner() == Thread.CurrentThread.GetSmallId(); + } + else if (lw.IsInflated) + { + return MonitorStatus.GetOwner(lw.GetInflatedLock().status) == Thread.CurrentThread.GetSmallId(); + } + return false; + } + + // true if obj is owned by any thread + public static bool HasOwner(object obj) + { + LockWord lw = GetLockWord(ref obj); + + if (lw.IsFlat) + return !lw.IsFree; + else if (lw.IsInflated) + { + return MonitorStatus.GetOwner(lw.GetInflatedLock().status) != 0; + } + + return false; + } } diff --git a/src/mono/mono/metadata/icall-def.h b/src/mono/mono/metadata/icall-def.h index 813fbe3571182a..4086acfe207848 100644 --- a/src/mono/mono/metadata/icall-def.h +++ b/src/mono/mono/metadata/icall-def.h @@ -579,8 +579,6 @@ HANDLES(MONIT_0, "Enter", ves_icall_System_Threading_Monitor_Monitor_Enter, void HANDLES(MONIT_1, "Exit", mono_monitor_exit_icall, void, 1, (MonoObject)) HANDLES(MONIT_2, "Monitor_pulse", ves_icall_System_Threading_Monitor_Monitor_pulse, void, 1, (MonoObject)) HANDLES(MONIT_3, "Monitor_pulse_all", ves_icall_System_Threading_Monitor_Monitor_pulse_all, void, 1, (MonoObject)) -HANDLES(MONIT_4, "Monitor_test_owner", ves_icall_System_Threading_Monitor_Monitor_test_owner, MonoBoolean, 1, (MonoObject)) -HANDLES(MONIT_5, "Monitor_test_synchronised", ves_icall_System_Threading_Monitor_Monitor_test_synchronised, MonoBoolean, 1, (MonoObject)) HANDLES(MONIT_7, "Monitor_wait", ves_icall_System_Threading_Monitor_Monitor_wait, MonoBoolean, 3, (MonoObject, guint32, MonoBoolean)) NOHANDLES(ICALL(MONIT_8, "get_LockContentionCount", ves_icall_System_Threading_Monitor_Monitor_LockContentionCount)) HANDLES(MONIT_9, "try_enter_with_atomic_var", ves_icall_System_Threading_Monitor_Monitor_try_enter_with_atomic_var, void, 4, (MonoObject, guint32, MonoBoolean, MonoBoolean_ref)) diff --git a/src/mono/mono/metadata/monitor.c b/src/mono/mono/metadata/monitor.c index 322ff8459cb3b6..13499b0de8b8e8 100644 --- a/src/mono/mono/metadata/monitor.c +++ b/src/mono/mono/metadata/monitor.c @@ -1247,46 +1247,6 @@ mono_monitor_enter_v4_fast (MonoObject *obj, MonoBoolean *lock_taken) return (guint32)res; } -MonoBoolean -ves_icall_System_Threading_Monitor_Monitor_test_owner (MonoObjectHandle obj_handle, MonoError* error) -{ - MonoObject* const obj = MONO_HANDLE_RAW (obj_handle); - - LockWord lw; - - LOCK_DEBUG (g_message ("%s: Testing if %p is owned by thread %d", __func__, obj, mono_thread_info_get_small_id())); - - lw.sync = obj->synchronisation; - - if (lock_word_is_flat (lw)) { - return lock_word_get_owner (lw) == mono_thread_info_get_small_id (); - } else if (lock_word_is_inflated (lw)) { - return mon_status_get_owner (lock_word_get_inflated_lock (lw)->status) == mono_thread_info_get_small_id (); - } - - return FALSE; -} - -MonoBoolean -ves_icall_System_Threading_Monitor_Monitor_test_synchronised (MonoObjectHandle obj_handle, MonoError* error) -{ - MonoObject* const obj = MONO_HANDLE_RAW (obj_handle); - - LockWord lw; - - LOCK_DEBUG (g_message("%s: (%d) Testing if %p is owned by any thread", __func__, mono_thread_info_get_small_id (), obj)); - - lw.sync = obj->synchronisation; - - if (lock_word_is_flat (lw)) { - return !lock_word_is_free (lw); - } else if (lock_word_is_inflated (lw)) { - return mon_status_get_owner (lock_word_get_inflated_lock (lw)->status) != 0; - } - - return FALSE; -} - /* All wait list manipulation in the pulse, pulseall and wait * functions happens while the monitor lock is held, so we don't need * any extra struct locking From c3c16cf16b13466ed965255875d1165cf2f7e863 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 11:44:07 -0500 Subject: [PATCH 05/43] use sequential layout for LockWord --- .../src/System/Threading/ObjectHeader.Mono.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index f9c665ef56981b..52f44cef24638b 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -16,7 +16,7 @@ internal static class ObjectHeader private unsafe struct Header { #region Keep in sync with src/native/public/mono/metadata/details/object-types.h - public IntPtr vtable; + public void* vtable; public IntPtr synchronization; // really a LockWord #endregion // keep in sync with src/native/public/mono/metadata/details/object-types.h } @@ -56,12 +56,11 @@ public static uint SetOwner (uint status, int owner) // // A union that contains either an uninflated lockword, or a pointer to a synchronization struct // - [StructLayout(LayoutKind.Explicit)] + [StructLayout(LayoutKind.Sequential)] private struct LockWord { #region Keep in sync with monitor.h - [FieldOffset(0)] private IntPtr _lock_word; private const int StatusBits = 2; From bca91132f948194dcca60ce8903a0f1e11b8bc98 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 11:45:52 -0500 Subject: [PATCH 06/43] remove managed Inflate (ifdef'd out) it needs at least alloc_mon/discard_mon icalls which might kill any advantage of having a purely managed path --- .../src/System/Threading/ObjectHeader.Mono.cs | 70 ------------------- 1 file changed, 70 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 52f44cef24638b..1faff19907292b 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -211,42 +211,6 @@ public static bool TryGetHashCode(object? o, out int hash) return false; } -#if false // need to implement alloc_mon/discard_mon - private static void Inflate(object o) - { - unsafe - { - MonoThreadsSync *mon = alloc_mon(); - LockWord nlw = LockWord.NewInflated (mon); - LockWord old_lw = GetLockWord (ref o); - while (true) - { - if (old_lw.IsInflated) - break; - else if (old_lw.HasHash) - { - mon->hash_code = old_lw.FlatHash; - mon->status = SyncSetOwner (mon->status, 0); - nlw = nlw.SetHasHash(); - - } - else if (old_lw.IsFree) - { - mon->status = SyncSetOwner (mon->status, old_lw.GetOwner()); - mon->nest = old_lw.FlatNest; - } - Interlocked.MemoryBarrier(); - IntPtr prev = LockWordCompareExchange(ref o, nlw, old_lw); - if (prev == old_lw.AsIntPtr) - return; // success - old_lw.SetFromIntPtr(prev); // go around one more time - } - // someone else inflated it first - discard_mon (mon); - } - } -#endif - private static bool TryEnterInflatedFast(object o, ref bool lockTaken) { LockWord lw = GetLockWord (ref o); @@ -303,11 +267,6 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) return true; } else { return false; -#if false - // someone acquired it in the meantime or put in a hash - Inflate(o); - return TryEnterInflatedFast(o, ref lockTaken); -#endif } } else if (lw.IsInflated) @@ -317,38 +276,9 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) else if (lw.IsFlat) { return false; -#if false - int owner = Thread.CurrentThread.GetSmallId(); - if (lw.GetOwner() == owner) - { - if (lw.IsMaxNest) - { - InflateOwned(o); - return TryEnterInflatedFast(o, ref lockTaken); - } - else - { - LockWord nlw = lw.IncrementNest(); - IntPtr prev = LockWordCompareExchange(ref o, nlw, lw); - if (prev != lw.AsIntPtr) - { - // Someone else inflated it in the meantime - return TryEnterInflatedFast(o, ref lockTaken); - } - lockTaken = true; - return true; - } - } - Inflate(o); - return TryEnterInflatedFast(o, refLockTaken); -#endif } Debug.Assert (lw.HasHash); return false; -#if false - Inflate(o); - return TryEnterInflatedFast(o, ref lockTaken); -#endif } // true if obj is owned by the current thread From cef418a41fd1f6ef2baa076c39391c3efd6112c4 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 12:07:38 -0500 Subject: [PATCH 07/43] access sync block through helpers --- .../src/System/Threading/ObjectHeader.Mono.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 1faff19907292b..a92210b586235a 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -33,6 +33,18 @@ private unsafe struct MonoThreadsSync #endregion // keep in sync with monitor.h } + private static class SyncBlock + { + public static ref int HashCode(ref MonoThreadsSync mon) => ref mon.hash_code; + public static ref uint Status (ref MonoThreadsSync mon) => ref mon.status; + + // only call if current thread owns the lock + public static void IncrementNest (ref MonoThreadsSync mon) + { + mon.nest++; + } + } + // // Manipulate the MonoThreadSync:status field // @@ -201,7 +213,7 @@ public static bool TryGetHashCode(object? o, out int hash) LockWord lw = GetLockWord (ref o); if (lw.HasHash) { if (lw.IsInflated) { - hash = lw.GetInflatedLock().hash_code; + hash = SyncBlock.HashCode(ref lw.GetInflatedLock()); return true; } else { hash = lw.FlatHash; @@ -218,11 +230,11 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) ref MonoThreadsSync mon = ref lw.GetInflatedLock(); while (true) { - uint old_status = mon.status; + uint old_status = SyncBlock.Status (ref mon); if (MonitorStatus.GetOwner(old_status) == 0) { uint new_status = MonitorStatus.SetOwner(old_status, small_id); - uint prev_status = Interlocked.CompareExchange (ref mon.status, new_status, old_status); + uint prev_status = Interlocked.CompareExchange (ref SyncBlock.Status (ref mon), new_status, old_status); if (prev_status == old_status) { lockTaken = true; @@ -234,7 +246,7 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) if (MonitorStatus.GetOwner(old_status) == small_id) { // we own it - mon.nest++; + SyncBlock.IncrementNest (ref mon); lockTaken = true; return true; } From a97a01fba3335fa8229df9e3b8b5de60b51c8731 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 12:57:44 -0500 Subject: [PATCH 08/43] add DumpRef to print out pointer values from ObjectHeader --- .../src/System/Threading/ObjectHeader.Mono.cs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index a92210b586235a..8235acb9df0b12 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -189,6 +189,7 @@ internal void SetFromIntPtr (IntPtr new_lw) private static LockWord GetLockWord(ref object obj) { ref Header h = ref Unsafe.As(ref obj); + DumpRef ("glw h", ref h); LockWord lw = LockWord.FromObjectHeader(ref h); GC.KeepAlive(obj); return lw; @@ -210,10 +211,16 @@ public static bool TryGetHashCode(object? o, out int hash) if (o == null) return true; + DumpRef ("tghc o", ref o); + LockWord lw = GetLockWord (ref o); if (lw.HasHash) { if (lw.IsInflated) { - hash = SyncBlock.HashCode(ref lw.GetInflatedLock()); + ref MonoThreadsSync mon = ref lw.GetInflatedLock(); + DumpRef ("tghc mon", ref mon); + ref int hashRef = ref SyncBlock.HashCode(ref mon); + DumpRef ("tghc hashRef", ref hashRef); + hash = hashRef; return true; } else { hash = lw.FlatHash; @@ -323,4 +330,13 @@ public static bool HasOwner(object obj) return false; } + + private static void DumpRef(string desc, scoped ref T thing) + { + unsafe + { + IntPtr p = (IntPtr)Unsafe.AsPointer (ref thing); + Debug.WriteLine ($"{desc} 0x{p:x}"); + } + } } From e810d94badd561deef37e27cfc50114bacfe23f1 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 14:49:57 -0500 Subject: [PATCH 09/43] Get a pointer to the object header another way Unsafe.As() was converting a object** into a header*. Go through Unsafe.AsPointer to get a Header** and then deref it once to get a header --- .../src/System/Threading/ObjectHeader.Mono.cs | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 8235acb9df0b12..68d5622c0c11f8 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -186,19 +186,32 @@ internal void SetFromIntPtr (IntPtr new_lw) } } + private static unsafe ref Header ObjectHeaderUNSAFE(ref object obj) + { + Header** hptr = (Header**)Unsafe.AsPointer(ref obj); + ref Header h = ref Unsafe.AsRef
(*hptr); + return ref h; + } + private static LockWord GetLockWord(ref object obj) { - ref Header h = ref Unsafe.As(ref obj); - DumpRef ("glw h", ref h); - LockWord lw = LockWord.FromObjectHeader(ref h); + LockWord lw; + unsafe + { + ref Header h = ref ObjectHeaderUNSAFE(ref obj); + DumpRef ("glw h", ref h); + lw = LockWord.FromObjectHeader(ref h); + } GC.KeepAlive(obj); return lw; } private static IntPtr LockWordCompareExchange (ref object obj, LockWord nlw, LockWord expected) { - ref Header h = ref Unsafe.As(ref obj); - return Interlocked.CompareExchange (ref h.synchronization, nlw.AsIntPtr, expected.AsIntPtr); + ref Header h = ref ObjectHeaderUNSAFE(ref obj); + IntPtr result = Interlocked.CompareExchange (ref h.synchronization, nlw.AsIntPtr, expected.AsIntPtr); + GC.KeepAlive (obj); + return result; } /// @@ -333,10 +346,12 @@ public static bool HasOwner(object obj) private static void DumpRef(string desc, scoped ref T thing) { +#if false unsafe { IntPtr p = (IntPtr)Unsafe.AsPointer (ref thing); - Debug.WriteLine ($"{desc} 0x{p:x}"); + Internal.Console.Error.Write ($"{desc} 0x{p:x}\n"); } +#endif } } From c51ff1c0537412d7afb9dc03834027ce2e46f54e Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 16:34:21 -0500 Subject: [PATCH 10/43] remove DumpRef --- .../src/System/Threading/ObjectHeader.Mono.cs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 68d5622c0c11f8..c3379f328f41bc 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -199,7 +199,6 @@ private static LockWord GetLockWord(ref object obj) unsafe { ref Header h = ref ObjectHeaderUNSAFE(ref obj); - DumpRef ("glw h", ref h); lw = LockWord.FromObjectHeader(ref h); } GC.KeepAlive(obj); @@ -224,15 +223,11 @@ public static bool TryGetHashCode(object? o, out int hash) if (o == null) return true; - DumpRef ("tghc o", ref o); - LockWord lw = GetLockWord (ref o); if (lw.HasHash) { if (lw.IsInflated) { ref MonoThreadsSync mon = ref lw.GetInflatedLock(); - DumpRef ("tghc mon", ref mon); ref int hashRef = ref SyncBlock.HashCode(ref mon); - DumpRef ("tghc hashRef", ref hashRef); hash = hashRef; return true; } else { @@ -344,14 +339,4 @@ public static bool HasOwner(object obj) return false; } - private static void DumpRef(string desc, scoped ref T thing) - { -#if false - unsafe - { - IntPtr p = (IntPtr)Unsafe.AsPointer (ref thing); - Internal.Console.Error.Write ($"{desc} 0x{p:x}\n"); - } -#endif - } } From 2939ac223cc3f05572faaf3054f33b8cb091165c Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 16:34:32 -0500 Subject: [PATCH 11/43] XXX temporary return from MonoResolveUnmanagedDll if context is null There's some kind of situation where apparently Default is seen to be null - so we got into a loop between the managed hash code implementation and the static constructor of AssemblyLoadContext --- .../src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs index 9032e77fc239d0..cd863197cd941c 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs @@ -159,6 +159,8 @@ private static AssemblyLoadContext GetAssemblyLoadContext(IntPtr gchManagedAssem private static void MonoResolveUnmanagedDll(string unmanagedDllName, IntPtr gchManagedAssemblyLoadContext, ref IntPtr dll) { AssemblyLoadContext context = GetAssemblyLoadContext(gchManagedAssemblyLoadContext); + if (context == null) + return; dll = context.LoadUnmanagedDll(unmanagedDllName); } From 6060694062258e636ac7f78151239e5c7930d2ac Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 16:43:11 -0500 Subject: [PATCH 12/43] XXX some bug in GetHashCode :-( --- .../src/System/Threading/ObjectHeader.Mono.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index c3379f328f41bc..8388774568634b 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -220,6 +220,7 @@ private static IntPtr LockWordCompareExchange (ref object obj, LockWord nlw, Loc public static bool TryGetHashCode(object? o, out int hash) { hash = 0; +#if false if (o == null) return true; @@ -235,6 +236,7 @@ public static bool TryGetHashCode(object? o, out int hash) return true; } } +#endif return false; } From 93ba644b3b5b591fd9a34dbb5a2d027179bf1917 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 16:52:39 -0500 Subject: [PATCH 13/43] BUGFIX: logical shifts, not arithmetic on wasm32, IntPtr and int are both the same size. so when we store a hash code into _lock_word that ends up with the 30th high bit set, the right shift to get it back out needs to be arithmetic or else we get incorrect hash codes --- .../src/System/Threading/ObjectHeader.Mono.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 8388774568634b..7ae506af41046f 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -117,7 +117,7 @@ public ref MonoThreadsSync GetInflatedLock() public bool IsNested => (_lock_word & NestMask) == NestMask; - public int FlatHash => (int)(_lock_word >> HashShift); + public int FlatHash => (int)(_lock_word >>> HashShift); public int FlatNest { @@ -126,7 +126,7 @@ public int FlatNest if (IsFree) return 0; /* Inword nest count starts from 0 */ - return 1 + (int)((_lock_word & NestMask) >> NestShift); + return 1 + (int)((_lock_word & NestMask) >>> NestShift); } } @@ -146,7 +146,7 @@ public LockWord DecrementNest() return res; } - public int GetOwner() => (int)(_lock_word >> OwnerShift); + public int GetOwner() => (int)(_lock_word >>> OwnerShift); public static LockWord NewThinHash(int hash) { From a9bd40efe7dd896c67b64f96d07a9bd9ec1bcca6 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 16:54:44 -0500 Subject: [PATCH 14/43] XXX disable hacks --- .../src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs | 2 ++ .../src/System/Threading/ObjectHeader.Mono.cs | 5 ++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs index cd863197cd941c..621105607aec3e 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs @@ -159,8 +159,10 @@ private static AssemblyLoadContext GetAssemblyLoadContext(IntPtr gchManagedAssem private static void MonoResolveUnmanagedDll(string unmanagedDllName, IntPtr gchManagedAssemblyLoadContext, ref IntPtr dll) { AssemblyLoadContext context = GetAssemblyLoadContext(gchManagedAssemblyLoadContext); +#if false if (context == null) return; +#endif dll = context.LoadUnmanagedDll(unmanagedDllName); } diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 7ae506af41046f..4ae377f113ac8a 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -220,7 +220,6 @@ private static IntPtr LockWordCompareExchange (ref object obj, LockWord nlw, Loc public static bool TryGetHashCode(object? o, out int hash) { hash = 0; -#if false if (o == null) return true; @@ -230,13 +229,13 @@ public static bool TryGetHashCode(object? o, out int hash) ref MonoThreadsSync mon = ref lw.GetInflatedLock(); ref int hashRef = ref SyncBlock.HashCode(ref mon); hash = hashRef; - return true; + return false; } else { hash = lw.FlatHash; return true; } } -#endif + GC.KeepAlive (o); return false; } From 129de8624932da193dc8177865a4835da8d59c87 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 16:58:09 -0500 Subject: [PATCH 15/43] put back the ALC hack --- .../src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs index 621105607aec3e..18f3d17deda5d5 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs @@ -159,7 +159,7 @@ private static AssemblyLoadContext GetAssemblyLoadContext(IntPtr gchManagedAssem private static void MonoResolveUnmanagedDll(string unmanagedDllName, IntPtr gchManagedAssemblyLoadContext, ref IntPtr dll) { AssemblyLoadContext context = GetAssemblyLoadContext(gchManagedAssemblyLoadContext); -#if false +#if true if (context == null) return; #endif From 7496a54879f7b35e3b6fad54d98ae5c4fbf6695e Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 17:45:59 -0500 Subject: [PATCH 16/43] force inlining --- .../src/System/Threading/ObjectHeader.Mono.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 4ae377f113ac8a..af64ae1e681a92 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -100,6 +100,7 @@ private enum Status public bool IsInflated => (_lock_word & (IntPtr)Status.Inflated) != 0; + [MethodImpl(MethodImplOptions.AggressiveInlining)] public ref MonoThreadsSync GetInflatedLock() { unsafe @@ -171,6 +172,7 @@ public static LockWord NewFlat(int owner) return res; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static LockWord FromObjectHeader(ref Header header) { LockWord lw; @@ -186,6 +188,7 @@ internal void SetFromIntPtr (IntPtr new_lw) } } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private static unsafe ref Header ObjectHeaderUNSAFE(ref object obj) { Header** hptr = (Header**)Unsafe.AsPointer(ref obj); @@ -193,6 +196,7 @@ private static unsafe ref Header ObjectHeaderUNSAFE(ref object obj) return ref h; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private static LockWord GetLockWord(ref object obj) { LockWord lw; @@ -217,6 +221,7 @@ private static IntPtr LockWordCompareExchange (ref object obj, LockWord nlw, Loc /// Tries to get the hash code from the object if it is /// already known and return true. Otherwise returns false. /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool TryGetHashCode(object? o, out int hash) { hash = 0; @@ -277,6 +282,7 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) // returns false if we should fall back to the slow path // returns true if we tried to enter // sets lockTaken to true if the lock was taken + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool TryEnterFast(object? o, ref bool lockTaken) { if (lockTaken || o == null) @@ -310,6 +316,7 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) } // true if obj is owned by the current thread + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsEntered(object obj) { LockWord lw = GetLockWord(ref obj); @@ -326,6 +333,7 @@ public static bool IsEntered(object obj) } // true if obj is owned by any thread + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool HasOwner(object obj) { LockWord lw = GetLockWord(ref obj); From 5845dacc33da42bdf6bea03f27d4d122386f98db Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 21:29:36 -0500 Subject: [PATCH 17/43] move fast path into ReliableEnterTimeout --- .../src/System/Threading/Monitor.Mono.cs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index 4148883307b3dc..7ec397b38b24fa 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -19,10 +19,6 @@ public static void Enter(object obj, ref bool lockTaken) if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); - // fast path - if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) - return; - ReliableEnterTimeout(obj, (int)Timeout.Infinite, ref lockTaken); } @@ -41,9 +37,6 @@ public static void TryEnter(object obj, ref bool lockTaken) if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); - if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) - return; - ReliableEnterTimeout(obj, 0, ref lockTaken); } @@ -60,9 +53,6 @@ public static void TryEnter(object obj, int millisecondsTimeout, ref bool lockTa if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); - if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) - return; - ReliableEnterTimeout(obj, millisecondsTimeout, ref lockTaken); } @@ -136,6 +126,10 @@ private static void ReliableEnterTimeout(object obj, int timeout, ref bool lockT if (timeout < 0 && timeout != (int)Timeout.Infinite) throw new ArgumentOutOfRangeException(nameof(timeout)); + // fast path + if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) + return; + try_enter_with_atomic_var(obj, timeout, true, ref lockTaken); } From 847382bf5166a98eb868db68d7ee8de13cb5cdc7 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 22:48:00 -0500 Subject: [PATCH 18/43] Don't pass lockTaken to ObjectHolder.TryEnterFast set it in the caller if we succeeded --- .../src/System/Threading/Monitor.Mono.cs | 4 +++- .../src/System/Threading/ObjectHeader.Mono.cs | 17 +++++------------ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index 7ec397b38b24fa..73592d08b0dfa0 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -127,8 +127,10 @@ private static void ReliableEnterTimeout(object obj, int timeout, ref bool lockT throw new ArgumentOutOfRangeException(nameof(timeout)); // fast path - if (ObjectHeader.TryEnterFast(obj, ref lockTaken)) + if (ObjectHeader.TryEnterFast(obj)) { + lockTaken = true; return; + } try_enter_with_atomic_var(obj, timeout, true, ref lockTaken); } diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index af64ae1e681a92..ff2776213fbbbd 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -244,7 +244,7 @@ public static bool TryGetHashCode(object? o, out int hash) return false; } - private static bool TryEnterInflatedFast(object o, ref bool lockTaken) + private static bool TryEnterInflatedFast(object o) { LockWord lw = GetLockWord (ref o); int small_id = Thread.CurrentThread.GetSmallId(); @@ -258,7 +258,6 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) uint prev_status = Interlocked.CompareExchange (ref SyncBlock.Status (ref mon), new_status, old_status); if (prev_status == old_status) { - lockTaken = true; return true; } // someone else changed the status, go around the loop again @@ -268,7 +267,6 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) { // we own it SyncBlock.IncrementNest (ref mon); - lockTaken = true; return true; } else @@ -280,15 +278,11 @@ private static bool TryEnterInflatedFast(object o, ref bool lockTaken) } // returns false if we should fall back to the slow path - // returns true if we tried to enter - // sets lockTaken to true if the lock was taken + // returns true if the lock was taken [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static bool TryEnterFast(object? o, ref bool lockTaken) + public static bool TryEnterFast(object? o) { - if (lockTaken || o == null) - return false; - - lockTaken = false; + Debug.Assert (o != null); LockWord lw = GetLockWord (ref o); if (lw.IsFree) @@ -297,7 +291,6 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) LockWord nlw = LockWord.NewFlat(owner); if (LockWordCompareExchange (ref o, nlw, lw) == lw.AsIntPtr) { - lockTaken = true; return true; } else { return false; @@ -305,7 +298,7 @@ public static bool TryEnterFast(object? o, ref bool lockTaken) } else if (lw.IsInflated) { - return TryEnterInflatedFast(o, ref lockTaken); + return TryEnterInflatedFast(o); } else if (lw.IsFlat) { From 7a356ab0ade8c16e5176cab3ebda60e3e2bb75ca Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 22:59:04 -0500 Subject: [PATCH 19/43] TryEnterFast: handle flat recursive locking, too --- .../src/System/Threading/ObjectHeader.Mono.cs | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index ff2776213fbbbd..7bbe2e6e416860 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -282,8 +282,7 @@ private static bool TryEnterInflatedFast(object o) [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool TryEnterFast(object? o) { - Debug.Assert (o != null); - + Debug.Assert (o != null); LockWord lw = GetLockWord (ref o); if (lw.IsFree) { @@ -302,6 +301,27 @@ public static bool TryEnterFast(object? o) } else if (lw.IsFlat) { + int owner = Thread.CurrentThread.GetSmallId(); + if (lw.GetOwner() == owner) + { + if (lw.IsNestMax) + { + // too much recursive locking, need to inflate + return false; + } else { + LockWord nlw = lw.IncrementNest(); + if (LockWordCompareExchange (ref o, nlw, lw) == lw.AsIntPtr) + { + return true; + } + else + { + // someone else inflated it in the meantime, fall back to slow path + return false; + } + } + } + // there's contention, go to slow path return false; } Debug.Assert (lw.HasHash); From 5fc1cdc2d85b9b675a50c1c7887625fa932f0780 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 23:01:58 -0500 Subject: [PATCH 20/43] whitespace --- .../src/System/Threading/Monitor.Mono.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index 73592d08b0dfa0..64fe62391d5990 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -128,9 +128,9 @@ private static void ReliableEnterTimeout(object obj, int timeout, ref bool lockT // fast path if (ObjectHeader.TryEnterFast(obj)) { - lockTaken = true; + lockTaken = true; return; - } + } try_enter_with_atomic_var(obj, timeout, true, ref lockTaken); } From bf088fdb730d0c2cd93272935589adfd989e0160 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 1 Feb 2023 23:28:55 -0500 Subject: [PATCH 21/43] Implement Monitor.Exit fast path in managed If the LockWord is flat, there has been no contention, so there is noone to signal when we exit --- .../src/System/Threading/Monitor.Mono.cs | 13 +++++++++- .../src/System/Threading/ObjectHeader.Mono.cs | 24 ++++++++++++++++++- src/mono/mono/metadata/icall-def.h | 2 +- 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index 64fe62391d5990..56395110ac593a 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -23,7 +23,18 @@ public static void Enter(object obj, ref bool lockTaken) } [MethodImplAttribute(MethodImplOptions.InternalCall)] - public static extern void Exit(object obj); + private static extern void InternalExit(object obj); + + public static void Exit(object obj) + { + ArgumentNullException.ThrowIfNull(obj); + if (!ObjectHeader.IsEntered(obj)) + throw new SynchronizationLockException(SR.Arg_SynchronizationLockException); + if (ObjectHeader.TryExit(obj)) + return; + + InternalExit(obj); + } public static bool TryEnter(object obj) { diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 7bbe2e6e416860..81b4b676a7ef41 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -116,7 +116,7 @@ public ref MonoThreadsSync GetInflatedLock() public bool IsFlat => (_lock_word & StatusMask) == (IntPtr)Status.Flat; - public bool IsNested => (_lock_word & NestMask) == NestMask; + public bool IsNested => (_lock_word & NestMask) != 0; public int FlatHash => (int)(_lock_word >>> HashShift); @@ -361,4 +361,26 @@ public static bool HasOwner(object obj) return false; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool TryExit(object obj) + { + LockWord lw = GetLockWord(ref obj); + + if (lw.IsInflated) + return false; // there might be waiters to wake + // if the lock word is flat, there has been no contention + LockWord nlw; + if (lw.IsNested) + nlw = lw.DecrementNest(); + else + nlw = default; + + if (LockWordCompareExchange (ref obj, nlw, lw) == lw.AsIntPtr) + return true; + // someone inflated the lock in the meantime, fall back to the slow path + + GC.KeepAlive(obj); + return false; + } + } diff --git a/src/mono/mono/metadata/icall-def.h b/src/mono/mono/metadata/icall-def.h index 4086acfe207848..464b708b6e3975 100644 --- a/src/mono/mono/metadata/icall-def.h +++ b/src/mono/mono/metadata/icall-def.h @@ -576,7 +576,7 @@ NOHANDLES(ICALL(LIFOSEM_4, "TimedWaitInternal", ves_icall_System_Threading_LowLe ICALL_TYPE(MONIT, "System.Threading.Monitor", MONIT_0) HANDLES(MONIT_0, "Enter", ves_icall_System_Threading_Monitor_Monitor_Enter, void, 1, (MonoObject)) -HANDLES(MONIT_1, "Exit", mono_monitor_exit_icall, void, 1, (MonoObject)) +HANDLES(MONIT_1, "InternalExit", mono_monitor_exit_icall, void, 1, (MonoObject)) HANDLES(MONIT_2, "Monitor_pulse", ves_icall_System_Threading_Monitor_Monitor_pulse, void, 1, (MonoObject)) HANDLES(MONIT_3, "Monitor_pulse_all", ves_icall_System_Threading_Monitor_Monitor_pulse_all, void, 1, (MonoObject)) HANDLES(MONIT_7, "Monitor_wait", ves_icall_System_Threading_Monitor_Monitor_wait, MonoBoolean, 3, (MonoObject, guint32, MonoBoolean)) From 1ac6759eab1f30ea0070a39073ffb80bba870417 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 2 Feb 2023 09:13:01 -0500 Subject: [PATCH 22/43] Don't call MonoResolveUnmanagedDll for the default ALC it would always return null anyway. But also this avoids a circular dependency between the managed Default ALC constructor and Monitor icalls (the ALC base constructor locks allContexts). --- .../System/Runtime/Loader/AssemblyLoadContext.Mono.cs | 4 ---- src/mono/mono/metadata/native-library.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs index 18f3d17deda5d5..9032e77fc239d0 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs @@ -159,10 +159,6 @@ private static AssemblyLoadContext GetAssemblyLoadContext(IntPtr gchManagedAssem private static void MonoResolveUnmanagedDll(string unmanagedDllName, IntPtr gchManagedAssemblyLoadContext, ref IntPtr dll) { AssemblyLoadContext context = GetAssemblyLoadContext(gchManagedAssemblyLoadContext); -#if true - if (context == null) - return; -#endif dll = context.LoadUnmanagedDll(unmanagedDllName); } diff --git a/src/mono/mono/metadata/native-library.c b/src/mono/mono/metadata/native-library.c index 164a3aef132f22..a519620215930c 100644 --- a/src/mono/mono/metadata/native-library.c +++ b/src/mono/mono/metadata/native-library.c @@ -706,6 +706,16 @@ netcore_resolve_with_load (MonoAssemblyLoadContext *alc, const char *scope, Mono if (mono_runtime_get_no_exec ()) return NULL; + /* default ALC LoadUnmanagedDll always returns null */ + /* NOTE: This is more than an optimization. It allows us to avoid triggering creation of + * the managed object for the Default ALC while we're looking up icalls for Monitor. The + * AssemblyLoadContext base constructor uses a lock on the allContexts variable which leads + * to recursive static constructor invocation which may show unexpected side effects --- + * such as a null AssemblyLoadContext.Default --- early in the runtime. + */ + if (mono_alc_is_default (alc)) + return NULL; + HANDLE_FUNCTION_ENTER (); MonoStringHandle scope_handle; From 60ba170b7a7f692a033c84a7effbf266f8d5f6a6 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 2 Feb 2023 09:29:00 -0500 Subject: [PATCH 23/43] [interp] don't lookup internal calls that were replaced by an opcode If it was replaced by an intrinsic, we're not really going to call the wrapper, no need to look for it --- src/mono/mono/mini/interp/transform.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index b1b819bf84941a..ad6c9be9c5c01f 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -3486,7 +3486,8 @@ interp_transform_call (TransformData *td, MonoMethod *method, MonoMethod *target } } - target_method = interp_transform_internal_calls (method, target_method, csignature, is_virtual); + if (op == -1) + target_method = interp_transform_internal_calls (method, target_method, csignature, is_virtual); if (csignature->call_convention == MONO_CALL_VARARG) csignature = mono_method_get_signature_checked (target_method, image, token, generic_context, error); From 8ecb7ff96b9c811755c3ed7571af07c7269304fb Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 2 Feb 2023 12:59:42 -0500 Subject: [PATCH 24/43] Add Interlocked.CompareExchange interp intrinsics for I4 and I8 --- src/mono/mono/mini/interp/interp.c | 35 +++++++++++++++++++++++++++ src/mono/mono/mini/interp/mintops.def | 2 ++ src/mono/mono/mini/interp/transform.c | 8 ++++++ 3 files changed, 45 insertions(+) diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index ab5b9d7281eff0..41efe38627f381 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -7007,6 +7007,41 @@ MINT_IN_CASE(MINT_BRTRUE_I8_SP) ZEROP_SP(gint64, !=); MINT_IN_BREAK; ip += 4; MINT_IN_BREAK; } + MINT_IN_CASE(MINT_MONO_CMPXCHG_I4) { + gint32 *dest = LOCAL_VAR(ip[2], gint32*); + gint32 value = LOCAL_VAR(ip[3], gint32); + gint32 comparand = LOCAL_VAR(ip[4], gint32); + NULL_CHECK(dest); + + LOCAL_VAR(ip[1], gint32) = mono_atomic_cas_i32(dest, value, comparand); + ip += 5; + MINT_IN_BREAK; + } + MINT_IN_CASE(MINT_MONO_CMPXCHG_I8) { + gboolean flag = FALSE; + gint64 *dest = LOCAL_VAR(ip[2], gint64*); + gint64 value = LOCAL_VAR(ip[3], gint64); + gint64 comparand = LOCAL_VAR(ip[4], gint64); + NULL_CHECK(dest); + +#if SIZEOF_VOID_P == 4 + if (G_UNLIKELY ((size_t)dest & 0x7)) { + gint64 old; + mono_interlocked_lock (); + old = *dest; + if (old == comparand) + *dest = value; + mono_interlocked_unlock (); + LOCAL_VAR(ip[1], gint64) = old; + flag = TRUE; + } +#endif + + if (!flag) + LOCAL_VAR(ip[1], gint64) = mono_atomic_cas_i64(dest, value, comparand); + ip += 5; + MINT_IN_BREAK; + } MINT_IN_CASE(MINT_MONO_LDDOMAIN) LOCAL_VAR (ip [1], gpointer) = mono_domain_get (); ip += 2; diff --git a/src/mono/mono/mini/interp/mintops.def b/src/mono/mono/mini/interp/mintops.def index c0034017930f89..f7634ce9806e2a 100644 --- a/src/mono/mono/mini/interp/mintops.def +++ b/src/mono/mono/mini/interp/mintops.def @@ -719,6 +719,8 @@ OPDEF(MINT_MONO_NEWOBJ, "mono_newobj", 3, 1, 0, MintOpClassToken) OPDEF(MINT_MONO_RETOBJ, "mono_retobj", 2, 0, 1, MintOpNoArgs) OPDEF(MINT_MONO_MEMORY_BARRIER, "mono_memory_barrier", 1, 0, 0, MintOpNoArgs) OPDEF(MINT_MONO_EXCHANGE_I8, "mono_interlocked.xchg.i8", 4, 1, 2, MintOpNoArgs) +OPDEF(MINT_MONO_CMPXCHG_I4, "mono_interlocked.cmpxchg.i4", 5, 1, 3, MintOpNoArgs) +OPDEF(MINT_MONO_CMPXCHG_I8, "mono_interlocked.cmpxchg.i8", 5, 1, 3, MintOpNoArgs) OPDEF(MINT_MONO_LDDOMAIN, "mono_lddomain", 2, 1, 0, MintOpNoArgs) OPDEF(MINT_MONO_ENABLE_GCTRANS, "mono_enable_gctrans", 1, 0, 0, MintOpNoArgs) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index ad6c9be9c5c01f..bfc67214fd328f 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -2555,6 +2555,14 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas *op = MINT_MONO_MEMORY_BARRIER; else if (!strcmp (tm, "Exchange") && csignature->param_count == 2 && csignature->params [0]->type == MONO_TYPE_I8 && csignature->params [1]->type == MONO_TYPE_I8) *op = MINT_MONO_EXCHANGE_I8; + else if (!strcmp (tm, "CompareExchange") && csignature->param_count == 3 && + (csignature->params[1]->type == MONO_TYPE_I4 || + csignature->params[1]->type == MONO_TYPE_I8)) { + if (csignature->params[1]->type == MONO_TYPE_I4) + *op = MINT_MONO_CMPXCHG_I4; + else + *op = MINT_MONO_CMPXCHG_I8; + } } else if (in_corlib && !strcmp (klass_name_space, "System.Threading") && !strcmp (klass_name, "Thread")) { if (!strcmp (tm, "MemoryBarrier") && csignature->param_count == 0) *op = MINT_MONO_MEMORY_BARRIER; From 14cd820147fd9a45a5f995e88e337094944d9ec4 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 2 Feb 2023 13:19:13 -0500 Subject: [PATCH 25/43] ask EE to inline LockWordCompareExchange --- .../src/System/Threading/ObjectHeader.Mono.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 81b4b676a7ef41..4ef5a71d640a5c 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -209,6 +209,7 @@ private static LockWord GetLockWord(ref object obj) return lw; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private static IntPtr LockWordCompareExchange (ref object obj, LockWord nlw, LockWord expected) { ref Header h = ref ObjectHeaderUNSAFE(ref obj); From 0360524fef65a0870b3e491c94fdd363c04f119c Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 2 Feb 2023 15:24:55 -0500 Subject: [PATCH 26/43] Use a ref struct to pass around the address of a MonoObject* on the stack This is similar to ObjectHandleOnStack except with a getter that lets us view the object header. Get rid of calls to GC.KeepAlive. With this, the fast path (locking a flat unowned object) doesn't have any calls on it besides argument validation --- .../src/System/Threading/ObjectHeader.Mono.cs | 77 ++++++++++--------- 1 file changed, 42 insertions(+), 35 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 4ef5a71d640a5c..37b01f27ae4195 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -21,6 +21,28 @@ private unsafe struct Header #endregion // keep in sync with src/native/public/mono/metadata/details/object-types.h } + // This is similar to QCallHandler ObjectHandleOnStack, but with a getter that let's view the + // object's header. This does two things: + // + // 1. It gives us a way to pass around a reference to the object header + // + // 2. because mono uses conservative stack scanning, we ensure there's always some place on the + // stack that stores a pointer to the object, thus pinning the object. + private unsafe ref struct ObjectHeaderOnStack + { + private Header** _header; + private ObjectHeaderOnStack(ref object o) + { + _header = (Header**)Unsafe.AsPointer(ref o); + } + public static ObjectHeaderOnStack Create(ref object o) + { + return new ObjectHeaderOnStack(ref o); + } + public ref Header Header => ref Unsafe.AsRef
(*_header); + + } + [StructLayout(LayoutKind.Sequential)] private unsafe struct MonoThreadsSync { @@ -189,33 +211,15 @@ internal void SetFromIntPtr (IntPtr new_lw) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static unsafe ref Header ObjectHeaderUNSAFE(ref object obj) + private static LockWord GetLockWord(ObjectHeaderOnStack h) { - Header** hptr = (Header**)Unsafe.AsPointer(ref obj); - ref Header h = ref Unsafe.AsRef
(*hptr); - return ref h; - } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static LockWord GetLockWord(ref object obj) - { - LockWord lw; - unsafe - { - ref Header h = ref ObjectHeaderUNSAFE(ref obj); - lw = LockWord.FromObjectHeader(ref h); - } - GC.KeepAlive(obj); - return lw; + return LockWord.FromObjectHeader(ref h.Header); } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static IntPtr LockWordCompareExchange (ref object obj, LockWord nlw, LockWord expected) + private static IntPtr LockWordCompareExchange (ObjectHeaderOnStack h, LockWord nlw, LockWord expected) { - ref Header h = ref ObjectHeaderUNSAFE(ref obj); - IntPtr result = Interlocked.CompareExchange (ref h.synchronization, nlw.AsIntPtr, expected.AsIntPtr); - GC.KeepAlive (obj); - return result; + return Interlocked.CompareExchange (ref h.Header.synchronization, nlw.AsIntPtr, expected.AsIntPtr); } /// @@ -229,7 +233,8 @@ public static bool TryGetHashCode(object? o, out int hash) if (o == null) return true; - LockWord lw = GetLockWord (ref o); + ObjectHeaderOnStack h = ObjectHeaderOnStack.Create(ref o); + LockWord lw = GetLockWord (h); if (lw.HasHash) { if (lw.IsInflated) { ref MonoThreadsSync mon = ref lw.GetInflatedLock(); @@ -241,13 +246,12 @@ public static bool TryGetHashCode(object? o, out int hash) return true; } } - GC.KeepAlive (o); return false; } - private static bool TryEnterInflatedFast(object o) + private static bool TryEnterInflatedFast(ObjectHeaderOnStack h) { - LockWord lw = GetLockWord (ref o); + LockWord lw = GetLockWord (h); int small_id = Thread.CurrentThread.GetSmallId(); ref MonoThreadsSync mon = ref lw.GetInflatedLock(); while (true) @@ -284,12 +288,13 @@ private static bool TryEnterInflatedFast(object o) public static bool TryEnterFast(object? o) { Debug.Assert (o != null); - LockWord lw = GetLockWord (ref o); + ObjectHeaderOnStack h = ObjectHeaderOnStack.Create (ref o); + LockWord lw = GetLockWord (h); if (lw.IsFree) { int owner = Thread.CurrentThread.GetSmallId(); LockWord nlw = LockWord.NewFlat(owner); - if (LockWordCompareExchange (ref o, nlw, lw) == lw.AsIntPtr) + if (LockWordCompareExchange (h, nlw, lw) == lw.AsIntPtr) { return true; } else { @@ -298,7 +303,7 @@ public static bool TryEnterFast(object? o) } else if (lw.IsInflated) { - return TryEnterInflatedFast(o); + return TryEnterInflatedFast(h); } else if (lw.IsFlat) { @@ -311,7 +316,7 @@ public static bool TryEnterFast(object? o) return false; } else { LockWord nlw = lw.IncrementNest(); - if (LockWordCompareExchange (ref o, nlw, lw) == lw.AsIntPtr) + if (LockWordCompareExchange (h, nlw, lw) == lw.AsIntPtr) { return true; } @@ -333,7 +338,8 @@ public static bool TryEnterFast(object? o) [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsEntered(object obj) { - LockWord lw = GetLockWord(ref obj); + ObjectHeaderOnStack h = ObjectHeaderOnStack.Create(ref obj); + LockWord lw = GetLockWord(h); if (lw.IsFlat) { @@ -350,7 +356,8 @@ public static bool IsEntered(object obj) [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool HasOwner(object obj) { - LockWord lw = GetLockWord(ref obj); + ObjectHeaderOnStack h = ObjectHeaderOnStack.Create(ref obj); + LockWord lw = GetLockWord(h); if (lw.IsFlat) return !lw.IsFree; @@ -365,7 +372,8 @@ public static bool HasOwner(object obj) [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool TryExit(object obj) { - LockWord lw = GetLockWord(ref obj); + ObjectHeaderOnStack h = ObjectHeaderOnStack.Create(ref obj); + LockWord lw = GetLockWord(h); if (lw.IsInflated) return false; // there might be waiters to wake @@ -376,11 +384,10 @@ public static bool TryExit(object obj) else nlw = default; - if (LockWordCompareExchange (ref obj, nlw, lw) == lw.AsIntPtr) + if (LockWordCompareExchange (h, nlw, lw) == lw.AsIntPtr) return true; // someone inflated the lock in the meantime, fall back to the slow path - GC.KeepAlive(obj); return false; } From bb1e69882529c31f681f03ec729b107455781215 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 6 Feb 2023 14:14:45 -0500 Subject: [PATCH 27/43] Add fast path for Monitor.Exit for inflated monitors with no waiters --- .../src/System/Threading/ObjectHeader.Mono.cs | 38 ++++++++++++++++++- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 37b01f27ae4195..de05a01c3a0542 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -65,6 +65,15 @@ public static void IncrementNest (ref MonoThreadsSync mon) { mon.nest++; } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool TryDecrementNest(ref MonoThreadsSync mon) + { + if (mon.nest == 0) + return false; + mon.nest--; + return true; + } } // @@ -75,7 +84,7 @@ private static class MonitorStatus #region Keep in sync with monitor.h private const uint OwnerMask = 0x0000ffffu; private const uint EntryCountMask = 0xffff0000u; - //private const uint EntryCountWaiters = 0x80000000u; + private const uint EntryCountWaiters = 0x80000000u; //private const uint EntryCountZero = 0x7fff0000u; //private const int EntryCountShift = 16; #endregion // keep in sync with monitor.h @@ -85,6 +94,8 @@ public static uint SetOwner (uint status, int owner) { return (status & EntryCountMask) | (uint)owner; } + + public static bool HaveWaiters(uint status) => (status & EntryCountWaiters) != 0; } // @@ -369,6 +380,29 @@ public static bool HasOwner(object obj) return false; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static bool TryExitInflated(ObjectHeaderOnStack h) + { + LockWord lw = GetLockWord(h); + ref MonoThreadsSync mon = ref lw.GetInflatedLock(); + + // if we're in a nested lock, decrement the count and we're done + if (SyncBlock.TryDecrementNest (ref mon)) + return true; + + ref uint status = ref SyncBlock.Status (ref mon); + uint old_status = status; + // if there are waiters, fall back to the slow path to wake them + if (MonitorStatus.HaveWaiters (old_status)) + return false; + uint new_status = MonitorStatus.SetOwner (old_status, 0); + uint prev_status = Interlocked.CompareExchange (ref status, new_status, old_status); + if (prev_status == old_status) + return true; // success, and there were no waiters, we're done + else + return false; // we need to retry, but maybe a waiter arrived, fall back to the slow path + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool TryExit(object obj) { @@ -376,7 +410,7 @@ public static bool TryExit(object obj) LockWord lw = GetLockWord(h); if (lw.IsInflated) - return false; // there might be waiters to wake + return TryExitInflated(h); // if the lock word is flat, there has been no contention LockWord nlw; if (lw.IsNested) From 6506515a1fd3327719485e213be74dc72db75f52 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 6 Feb 2023 14:15:17 -0500 Subject: [PATCH 28/43] Check for obj == null before ThrowIfNull In the interp it's cheaper to do a null check followed by a call than an unconditional call --- .../src/System/Threading/Monitor.Mono.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index 56395110ac593a..2632da49aa32dd 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -27,7 +27,8 @@ public static void Enter(object obj, ref bool lockTaken) public static void Exit(object obj) { - ArgumentNullException.ThrowIfNull(obj); + if (obj == null) + ArgumentNullException.ThrowIfNull(obj); if (!ObjectHeader.IsEntered(obj)) throw new SynchronizationLockException(SR.Arg_SynchronizationLockException); if (ObjectHeader.TryExit(obj)) @@ -130,9 +131,11 @@ private static bool ObjWait(int millisecondsTimeout, object obj) [MethodImplAttribute(MethodImplOptions.InternalCall)] internal static extern void try_enter_with_atomic_var(object obj, int millisecondsTimeout, bool allowInterruption, ref bool lockTaken); + [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void ReliableEnterTimeout(object obj, int timeout, ref bool lockTaken) { - ArgumentNullException.ThrowIfNull(obj); + if (obj == null) + ArgumentNullException.ThrowIfNull(obj); if (timeout < 0 && timeout != (int)Timeout.Infinite) throw new ArgumentOutOfRangeException(nameof(timeout)); From 91446f9547625ec1f726cbf52795df95927ba5dc Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 6 Feb 2023 14:16:00 -0500 Subject: [PATCH 29/43] Inline RuntimeHelpers.GetHashCode --- .../src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs index 3e3ca4db557cc4..c29743fb6c4ad5 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs @@ -39,6 +39,7 @@ public static int OffsetToStringData [MethodImplAttribute(MethodImplOptions.InternalCall)] private static extern int InternalGetHashCode(object? o); + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int GetHashCode(object? o) { if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) @@ -57,6 +58,7 @@ public static int GetHashCode(object? o) /// The advantage of this over is that it avoids assigning a hash /// code to the object if it does not already have one. /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static int TryGetHashCode(object? o) { if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) From 76805ba5e67301cef6ba5e7942c7a1af38d7bfe9 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 6 Feb 2023 15:44:00 -0500 Subject: [PATCH 30/43] inline TryEnterInflatedFast; nano-opts - change SyncBlock.HashCode to return the value, not a ref - we don't have managed code that writes into the sync block. - change TryEnterInflatedFast to avoid looping - if the CompareExchange fails, fall back to the slow path --- .../src/System/Threading/ObjectHeader.Mono.cs | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index de05a01c3a0542..fe8665cbdaae9d 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -57,7 +57,7 @@ private unsafe struct MonoThreadsSync private static class SyncBlock { - public static ref int HashCode(ref MonoThreadsSync mon) => ref mon.hash_code; + public static int HashCode(ref MonoThreadsSync mon) => mon.hash_code; public static ref uint Status (ref MonoThreadsSync mon) => ref mon.status; // only call if current thread owns the lock @@ -249,8 +249,7 @@ public static bool TryGetHashCode(object? o, out int hash) if (lw.HasHash) { if (lw.IsInflated) { ref MonoThreadsSync mon = ref lw.GetInflatedLock(); - ref int hashRef = ref SyncBlock.HashCode(ref mon); - hash = hashRef; + hash = SyncBlock.HashCode(ref mon); return false; } else { hash = lw.FlatHash; @@ -260,36 +259,35 @@ public static bool TryGetHashCode(object? o, out int hash) return false; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private static bool TryEnterInflatedFast(ObjectHeaderOnStack h) { LockWord lw = GetLockWord (h); int small_id = Thread.CurrentThread.GetSmallId(); ref MonoThreadsSync mon = ref lw.GetInflatedLock(); - while (true) + + uint old_status = SyncBlock.Status (ref mon); + if (MonitorStatus.GetOwner(old_status) == 0) { - uint old_status = SyncBlock.Status (ref mon); - if (MonitorStatus.GetOwner(old_status) == 0) + uint new_status = MonitorStatus.SetOwner(old_status, small_id); + uint prev_status = Interlocked.CompareExchange (ref SyncBlock.Status (ref mon), new_status, old_status); + if (prev_status == old_status) { - uint new_status = MonitorStatus.SetOwner(old_status, small_id); - uint prev_status = Interlocked.CompareExchange (ref SyncBlock.Status (ref mon), new_status, old_status); - if (prev_status == old_status) - { - return true; - } - // someone else changed the status, go around the loop again - continue; - } - if (MonitorStatus.GetOwner(old_status) == small_id) - { - // we own it - SyncBlock.IncrementNest (ref mon); return true; } - else - { - // someone else owns it, fall back to slow path - return false; - } + // someone else changed the status, fall back to the slow path + return false; + } + if (MonitorStatus.GetOwner(old_status) == small_id) + { + // we own it + SyncBlock.IncrementNest (ref mon); + return true; + } + else + { + // someone else owns it, fall back to slow path + return false; } } From 064eb10a136cc0e7a56e1f176dc118c7f0fc4c33 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 6 Feb 2023 16:01:11 -0500 Subject: [PATCH 31/43] disable the TryGetHashCode fast path helper It was actually making things slower on the interpreter. In the JIT it made object.GetHashCode about 50% faster for an object with a pre-computed hash, but that might mean we need the same kind of "no-wrapper; then wrapper" fastpath/slowpath that we have for the Monitor.Enter intrinsic in mini. --- .../System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs index c29743fb6c4ad5..fe012938bd279e 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs @@ -42,8 +42,10 @@ public static int OffsetToStringData [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int GetHashCode(object? o) { +#if false if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) return hash; +#endif return InternalGetHashCode(o); } @@ -61,8 +63,10 @@ public static int GetHashCode(object? o) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static int TryGetHashCode(object? o) { +#if false if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) return hash; +#endif return InternalTryGetHashCode(o); } From bbe5cde4d74c5aa2d99a76b5c20bb30f07b07d0a Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 6 Feb 2023 18:25:33 -0500 Subject: [PATCH 32/43] Fix last exit from inflated monitor When nest == 1 the caller is responsible for setting the owner to 0 to indicate that the monitor is unlocked. (But we leave the nest count == 1, so that it is correct next time the monitor is entered) --- .../src/System/Threading/ObjectHeader.Mono.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index fe8665cbdaae9d..9ae54cf2d999bb 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -69,8 +69,13 @@ public static void IncrementNest (ref MonoThreadsSync mon) [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool TryDecrementNest(ref MonoThreadsSync mon) { - if (mon.nest == 0) + Debug.Assert (mon.nest > 0); + if (mon.nest == 1) + { + // leave mon.nest == 1, the caller will set mon.owner to 0 to indicate the monitor + // is unlocked. nest will start from 1 for the next time it is entered. return false; + } mon.nest--; return true; } From cabd86434b8f9bbbf930eff8e1450df57b688933 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 7 Feb 2023 11:08:33 -0500 Subject: [PATCH 33/43] avoid repeated calls in TryEnterInflatedFast --- .../src/System/Threading/ObjectHeader.Mono.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index 9ae54cf2d999bb..e65918cefdc0f4 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -265,11 +265,8 @@ public static bool TryGetHashCode(object? o, out int hash) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static bool TryEnterInflatedFast(ObjectHeaderOnStack h) + private static bool TryEnterInflatedFast(scoped ref MonoThreadsSync mon, int small_id) { - LockWord lw = GetLockWord (h); - int small_id = Thread.CurrentThread.GetSmallId(); - ref MonoThreadsSync mon = ref lw.GetInflatedLock(); uint old_status = SyncBlock.Status (ref mon); if (MonitorStatus.GetOwner(old_status) == 0) @@ -304,9 +301,11 @@ public static bool TryEnterFast(object? o) Debug.Assert (o != null); ObjectHeaderOnStack h = ObjectHeaderOnStack.Create (ref o); LockWord lw = GetLockWord (h); + if (!lw.IsInflated && lw.HasHash) + return false; // need to inflate, fall back to native + int owner = Thread.CurrentThread.GetSmallId(); if (lw.IsFree) { - int owner = Thread.CurrentThread.GetSmallId(); LockWord nlw = LockWord.NewFlat(owner); if (LockWordCompareExchange (h, nlw, lw) == lw.AsIntPtr) { @@ -317,11 +316,10 @@ public static bool TryEnterFast(object? o) } else if (lw.IsInflated) { - return TryEnterInflatedFast(h); + return TryEnterInflatedFast(ref lw.GetInflatedLock(), owner); } else if (lw.IsFlat) { - int owner = Thread.CurrentThread.GetSmallId(); if (lw.GetOwner() == owner) { if (lw.IsNestMax) From f8da57da0a811c0fcf0d123c8ad5f79fc5ea212d Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 7 Feb 2023 12:14:42 -0500 Subject: [PATCH 34/43] Combine TryExit and IsEntered into TryExitChecked avoid duplicated lockword and sync block manipulations --- .../src/System/Threading/Monitor.Mono.cs | 4 +- .../src/System/Threading/ObjectHeader.Mono.cs | 40 ++++++++++++++----- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index 2632da49aa32dd..f9801fa36d16b0 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -29,9 +29,7 @@ public static void Exit(object obj) { if (obj == null) ArgumentNullException.ThrowIfNull(obj); - if (!ObjectHeader.IsEntered(obj)) - throw new SynchronizationLockException(SR.Arg_SynchronizationLockException); - if (ObjectHeader.TryExit(obj)) + if (ObjectHeader.TryExitChecked(obj)) return; InternalExit(obj); diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs index e65918cefdc0f4..16eccb9511c571 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/ObjectHeader.Mono.cs @@ -382,11 +382,8 @@ public static bool HasOwner(object obj) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static bool TryExitInflated(ObjectHeaderOnStack h) + private static bool TryExitInflated(scoped ref MonoThreadsSync mon) { - LockWord lw = GetLockWord(h); - ref MonoThreadsSync mon = ref lw.GetInflatedLock(); - // if we're in a nested lock, decrement the count and we're done if (SyncBlock.TryDecrementNest (ref mon)) return true; @@ -405,13 +402,10 @@ private static bool TryExitInflated(ObjectHeaderOnStack h) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static bool TryExit(object obj) + private static bool TryExitFlat(ObjectHeaderOnStack h, LockWord lw) { - ObjectHeaderOnStack h = ObjectHeaderOnStack.Create(ref obj); - LockWord lw = GetLockWord(h); - if (lw.IsInflated) - return TryExitInflated(h); + Debug.Assert (!lw.IsInflated); // if the lock word is flat, there has been no contention LockWord nlw; if (lw.IsNested) @@ -426,4 +420,32 @@ public static bool TryExit(object obj) return false; } + + // checks that obj is locked by the current thread + [MethodImpl(MethodImplOptions.AggressiveInlining)] + #pragma warning disable IDE0060 // spurious "unused parameter" warning because we never use obj, just take aref to it. + public static bool TryExitChecked(object obj) + #pragma warning restore IDE0060 + { + ObjectHeaderOnStack h = ObjectHeaderOnStack.Create(ref obj); + LockWord lw = GetLockWord(h); + bool owned = false; + + ref MonoThreadsSync mon = ref Unsafe.NullRef(); + if (lw.IsFlat) + { + owned = (lw.GetOwner() == Thread.CurrentThread.GetSmallId()); + } + else if (lw.IsInflated) + { + mon = ref lw.GetInflatedLock(); + owned = (MonitorStatus.GetOwner(mon.status) == Thread.CurrentThread.GetSmallId()); + } + if (!owned) + throw new SynchronizationLockException(SR.Arg_SynchronizationLockException); + if (lw.IsInflated) + return TryExitInflated(ref mon); + else + return TryExitFlat(h, lw); + } } From 2204a8230d539be17169022f0113a546b88202bb Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 7 Feb 2023 12:15:58 -0500 Subject: [PATCH 35/43] Re-enable managed GetHashCode in JIT; intrinsify in interp Instead of treating InternalGetHashCode/InternalTryGetHashCode as intrinsics in the interpreter, intrinsify RuntimeHelpers.GetHashCode RuntimeHelpers.TryGetHashCode and avoid the managed code entirely when possible --- .../System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs | 6 ++---- src/mono/mono/mini/interp/transform.c | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs index fe012938bd279e..732ebb26d8922a 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs @@ -42,10 +42,9 @@ public static int OffsetToStringData [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int GetHashCode(object? o) { -#if false + // NOTE: the interpreter does not run this code. It intrinsifies the whole RuntimeHelpers.GetHashCode function if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) return hash; -#endif return InternalGetHashCode(o); } @@ -63,10 +62,9 @@ public static int GetHashCode(object? o) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static int TryGetHashCode(object? o) { -#if false + // NOTE: the interpreter does not run this code. It intrinsifies the whole RuntimeHelpers.TryGetHashCode function if (Threading.ObjectHeader.TryGetHashCode (o, out int hash)) return hash; -#endif return InternalTryGetHashCode(o); } diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index bfc67214fd328f..97dd9ae3c5d9ee 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -2396,9 +2396,9 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas interp_ins_set_dreg (td->last_ins, td->sp [-1].local); td->ip += 5; return TRUE; - } else if (!strcmp (tm, "InternalGetHashCode")) { + } else if (!strcmp (tm, "GetHashCode") || !strcmp (tm, "InternalGetHashCode")) { *op = MINT_INTRINS_GET_HASHCODE; - } else if (!strcmp (tm, "InternalTryGetHashCode")) { + } else if (!strcmp (tm, "TryGetHashCode") || !strcmp (tm, "InternalTryGetHashCode")) { *op = MINT_INTRINS_TRY_GET_HASHCODE; } else if (!strcmp (tm, "GetRawData")) { interp_add_ins (td, MINT_LDFLDA_UNSAFE); From 47f2135aed46721438d6ef5335e0b0da715575b1 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 7 Feb 2023 13:25:15 -0500 Subject: [PATCH 36/43] [coop][interp] Fix GC transitions for thunk invoke wrappers The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms) --- src/mono/mono/mini/interp/transform.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index 97dd9ae3c5d9ee..8e23e6ab5c9938 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -7533,13 +7533,27 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, MonoJitICallId const jit_icall_id = (MonoJitICallId)read32 (td->ip + 1); MonoJitICallInfo const * const info = mono_find_jit_icall_info (jit_icall_id); - if (info->sig->ret->type != MONO_TYPE_VOID) { + // mono_method_get_unmanaged_thunk creates wrappers with + // enter_gc_unsafe/exit_gc_unsafe transitions. Treat them as attach/detach. + if (jit_icall_id == MONO_JIT_ICALL_mono_threads_enter_gc_unsafe_region_unbalanced) { + rtm->needs_thread_attach = 1; + // Add dummy return value + interp_add_ins (td, MINT_LDNULL); + push_simple_type (td, STACK_TYPE_I); + td->last_ins->dreg = td->sp[-1].local; + } else if (jit_icall_id == MONO_JIT_ICALL_mono_threads_exit_gc_unsafe_region_unbalanced) { + g_assert (rtm->needs_thread_attach); + // pop the unused gc var + td->sp--; + } else if (info->sig->ret->type != MONO_TYPE_VOID) { + g_assert_checked (jit_icall_id == MONO_JIT_ICALL_mono_threads_enter_gc_safe_region_unbalanced); // Push a dummy coop gc var interp_add_ins (td, MINT_LDNULL); push_simple_type (td, STACK_TYPE_I); td->last_ins->dreg = td->sp [-1].local; interp_add_ins (td, MINT_MONO_ENABLE_GCTRANS); } else { + g_assert_checked (jit_icall_id == MONO_JIT_ICALL_mono_threads_exit_gc_safe_region_unbalanced); // Pop the unused gc var td->sp--; } From fae8a2f8d124c57cba044f9508965a363ef0b2a6 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 8 Feb 2023 16:47:59 -0500 Subject: [PATCH 37/43] add CMPXCHG opcodes to the jiterpreter For the i64 version, pass the address of the i64 values to the helper function. The issue is that since JS doesn't have i64, EMSCRIPTEN_KEEPALIVE messes with the function's signature and we get import errors in the JITed wasm module. Passing by address works around the issue since addresses are i32. --- src/mono/mono/mini/interp/jiterpreter.c | 12 ++++++++++++ .../runtime/jiterpreter-trace-generator.ts | 19 +++++++++++++++++++ src/mono/wasm/runtime/jiterpreter.ts | 17 +++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/src/mono/mono/mini/interp/jiterpreter.c b/src/mono/mono/mini/interp/jiterpreter.c index 4b7647790d412f..c6c40d45482872 100644 --- a/src/mono/mono/mini/interp/jiterpreter.c +++ b/src/mono/mono/mini/interp/jiterpreter.c @@ -657,6 +657,18 @@ mono_jiterp_interp_entry_prologue (JiterpEntryData *data, void *this_arg) return sp_args; } +EMSCRIPTEN_KEEPALIVE int32_t +mono_jiterp_cas_i32 (volatile int32_t *addr, int32_t newVal, int32_t expected) +{ + return mono_atomic_cas_i32 (addr, newVal, expected); +} + +EMSCRIPTEN_KEEPALIVE void +mono_jiterp_cas_i64 (volatile int64_t *addr, int64_t *newVal, int64_t *expected, int64_t *oldVal) +{ + *oldVal= mono_atomic_cas_i64 (addr, *newVal, *expected); +} + // should_abort_trace returns one of these codes depending on the opcode and current state #define TRACE_IGNORE -1 #define TRACE_CONTINUE 0 diff --git a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts index 35959b91942e92..e93610bc4bb05e 100644 --- a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts +++ b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts @@ -770,6 +770,25 @@ export function generate_wasm_body ( append_stloc_tail(builder, getArgU16(ip, 1), isI32 ? WasmOpcode.i32_store : WasmOpcode.i64_store); break; } + case MintOpcode.MINT_MONO_CMPXCHG_I4: + builder.local("pLocals"); + append_ldloc(builder, getArgU16(ip, 2), WasmOpcode.i32_load); // dest + append_ldloc(builder, getArgU16(ip, 3), WasmOpcode.i32_load); // newVal + append_ldloc(builder, getArgU16(ip, 4), WasmOpcode.i32_load); // expected + builder.callImport("cmpxchg_i32"); + append_stloc_tail(builder, getArgU16(ip, 1), WasmOpcode.i32_store); + break; + case MintOpcode.MINT_MONO_CMPXCHG_I8: + // because i64 values can't pass through JS cleanly (c.f getRawCwrap and + // EMSCRIPTEN_KEEPALIVE), we pass addresses of newVal, expected and the return value + // to the helper function. The "dest" for the compare-exchange is already a + // pointer, so load it normally + append_ldloc(builder, getArgU16(ip, 2), WasmOpcode.i32_load); // dest + append_ldloca(builder, getArgU16(ip, 3), 0); // newVal + append_ldloca(builder, getArgU16(ip, 4), 0); // expected + append_ldloca(builder, getArgU16(ip, 1), 8, true); // oldVal + builder.callImport("cmpxchg_i64"); + break; default: if ( diff --git a/src/mono/wasm/runtime/jiterpreter.ts b/src/mono/wasm/runtime/jiterpreter.ts index 0455fcb4cc9306..7ee521eec603f6 100644 --- a/src/mono/wasm/runtime/jiterpreter.ts +++ b/src/mono/wasm/runtime/jiterpreter.ts @@ -260,6 +260,8 @@ function getTraceImports () { ["hasflag", "hasflag", getRawCwrap("mono_jiterp_enum_hasflag")], ["array_rank", "array_rank", getRawCwrap("mono_jiterp_get_array_rank")], ["stfld_o", "stfld_o", getRawCwrap("mono_jiterp_set_object_field")], + importDef("cmpxchg_i32", getRawCwrap("mono_jiterp_cas_i32")), + importDef("cmpxchg_i64", getRawCwrap("mono_jiterp_cas_i64")), ]; if (instrumentedMethodNames.length > 0) { @@ -523,6 +525,21 @@ function initialize_builder (builder: WasmBuilder) { "traceIp": WasmValtype.i32, }, WasmValtype.void, true ); + builder.defineType( + "cmpxchg_i32", { + "dest": WasmValtype.i32, + "newVal": WasmValtype.i32, + "expected": WasmValtype.i32, + }, WasmValtype.i32, true + ); + builder.defineType( + "cmpxchg_i64", { + "dest": WasmValtype.i32, + "newVal": WasmValtype.i32, + "expected": WasmValtype.i32, + "oldVal": WasmValtype.i32, + }, WasmValtype.void, true + ); } function assert_not_null ( From 1c0f6aebad00d83fb793cdba796cd1d9a39af5d1 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Feb 2023 14:38:51 -0500 Subject: [PATCH 38/43] Revert "[coop][interp] Fix GC transitions for thunk invoke wrappers" This reverts commit 5b498b8d4e265f8a68fe9453b535e48cf56756ce. --- src/mono/mono/mini/interp/transform.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index 8e23e6ab5c9938..97dd9ae3c5d9ee 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -7533,27 +7533,13 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, MonoJitICallId const jit_icall_id = (MonoJitICallId)read32 (td->ip + 1); MonoJitICallInfo const * const info = mono_find_jit_icall_info (jit_icall_id); - // mono_method_get_unmanaged_thunk creates wrappers with - // enter_gc_unsafe/exit_gc_unsafe transitions. Treat them as attach/detach. - if (jit_icall_id == MONO_JIT_ICALL_mono_threads_enter_gc_unsafe_region_unbalanced) { - rtm->needs_thread_attach = 1; - // Add dummy return value - interp_add_ins (td, MINT_LDNULL); - push_simple_type (td, STACK_TYPE_I); - td->last_ins->dreg = td->sp[-1].local; - } else if (jit_icall_id == MONO_JIT_ICALL_mono_threads_exit_gc_unsafe_region_unbalanced) { - g_assert (rtm->needs_thread_attach); - // pop the unused gc var - td->sp--; - } else if (info->sig->ret->type != MONO_TYPE_VOID) { - g_assert_checked (jit_icall_id == MONO_JIT_ICALL_mono_threads_enter_gc_safe_region_unbalanced); + if (info->sig->ret->type != MONO_TYPE_VOID) { // Push a dummy coop gc var interp_add_ins (td, MINT_LDNULL); push_simple_type (td, STACK_TYPE_I); td->last_ins->dreg = td->sp [-1].local; interp_add_ins (td, MINT_MONO_ENABLE_GCTRANS); } else { - g_assert_checked (jit_icall_id == MONO_JIT_ICALL_mono_threads_exit_gc_safe_region_unbalanced); // Pop the unused gc var td->sp--; } From 921cb08bb0814ea771b90aa2f62f0ac347c5eec5 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Feb 2023 10:17:35 -0500 Subject: [PATCH 39/43] Assert that CEE_MONO_GET_SP is followed by gc safe enter/exit icalls Only assert in debug builds --- src/mono/mono/mini/interp/transform.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index 97dd9ae3c5d9ee..f240fb3d97e282 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -7534,12 +7534,14 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, MonoJitICallInfo const * const info = mono_find_jit_icall_info (jit_icall_id); if (info->sig->ret->type != MONO_TYPE_VOID) { + g_assert_checked (jit_icall_id == MONO_JIT_ICALL_mono_threads_enter_gc_safe_region_unbalanced); // Push a dummy coop gc var interp_add_ins (td, MINT_LDNULL); push_simple_type (td, STACK_TYPE_I); td->last_ins->dreg = td->sp [-1].local; interp_add_ins (td, MINT_MONO_ENABLE_GCTRANS); } else { + g_assert_checked (jit_icall_id == MONO_JIT_ICALL_mono_threads_exit_gc_safe_region_unbalanced); // Pop the unused gc var td->sp--; } From 7a55bf0d0c20fe55c3fd46924a158d7bfbcba540 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 10 Feb 2023 11:39:16 -0500 Subject: [PATCH 40/43] [marshal] Add GCUnsafeTransitionBuilder; use for thunk_invoke_wrapper Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe. --- src/mono/mono/metadata/marshal-lightweight.c | 133 ++++++++++++++----- 1 file changed, 99 insertions(+), 34 deletions(-) diff --git a/src/mono/mono/metadata/marshal-lightweight.c b/src/mono/mono/metadata/marshal-lightweight.c index eff038a5839654..9d43d0b124d52e 100644 --- a/src/mono/mono/metadata/marshal-lightweight.c +++ b/src/mono/mono/metadata/marshal-lightweight.c @@ -696,6 +696,87 @@ gc_safe_transition_builder_cleanup (GCSafeTransitionBuilder *builder) #endif } +typedef struct EmitGCUnsafeTransitionBuilder { + MonoMethodBuilder *mb; + int orig_domain_var; + int attach_cookie_var; +} GCUnsafeTransitionBuilder; + +static void +gc_unsafe_transition_builder_init (GCUnsafeTransitionBuilder *builder, MonoMethodBuilder *mb, gboolean use_attach) +{ + g_assert_checked (use_attach); + // Right now we always set use_attach and use mono_threads_coop_attach to enter into gc + // unsafe regions. If !use_attach is needed (ie adding transitions, using + // mono_threads_enter_gc_unsafe_region_unbalanced) that needs to be implemented. + builder->mb = mb; + builder->orig_domain_var = -1; + builder->attach_cookie_var = -1; +} + +static void +gc_unsafe_transition_builder_add_vars (GCUnsafeTransitionBuilder *builder) +{ + MonoType *int_type = mono_get_int_type (); + builder->orig_domain_var = mono_mb_add_local (builder->mb, int_type); + builder->attach_cookie_var = mono_mb_add_local (builder->mb, int_type); +} + +static void +gc_unsafe_transition_builder_emit_enter (GCUnsafeTransitionBuilder *builder) +{ + MonoMethodBuilder *mb = builder->mb; + int attach_cookie = builder->attach_cookie_var; + int orig_domain = builder->orig_domain_var; + /* + * // does (STARTING|RUNNING|BLOCKING) -> RUNNING + set/switch domain + * intptr_t attach_cookie; + * intptr_t orig_domain = mono_threads_attach_coop (domain, &attach_cookie); + * + */ + /* orig_domain = mono_threads_attach_coop (domain, &attach_cookie); */ + mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX); + mono_mb_emit_byte (mb, CEE_MONO_LDDOMAIN); + mono_mb_emit_ldloc_addr (mb, attach_cookie); + /* + * This icall is special cased in the JIT so it works in native-to-managed wrappers in unattached threads. + * Keep this in sync with the CEE_JIT_ICALL code in the JIT. + * + * Special cased in interpreter, keep in sync. + */ + mono_mb_emit_icall (mb, mono_threads_attach_coop); + mono_mb_emit_stloc (mb, orig_domain); + + /* */ + emit_thread_interrupt_checkpoint (mb); +} + +static void +gc_unsafe_transition_builder_emit_exit (GCUnsafeTransitionBuilder *builder) +{ + MonoMethodBuilder *mb = builder->mb; + int orig_domain = builder->orig_domain_var; + int attach_cookie = builder->attach_cookie_var; + /* + * // does RUNNING -> (RUNNING|BLOCKING) + unset/switch domain + * mono_threads_detach_coop (orig_domain, &attach_cookie); + */ + + /* mono_threads_detach_coop (orig_domain, &attach_cookie); */ + mono_mb_emit_ldloc (mb, orig_domain); + mono_mb_emit_ldloc_addr (mb, attach_cookie); + /* Special cased in interpreter, keep in sync */ + mono_mb_emit_icall (mb, mono_threads_detach_coop); +} + +static void +gc_unsafe_transition_builder_cleanup (GCUnsafeTransitionBuilder *builder) +{ + builder->mb = NULL; + builder->orig_domain_var = -1; + builder->attach_cookie_var = -1; +} + static gboolean emit_native_wrapper_validate_signature (MonoMethodBuilder *mb, MonoMethodSignature* sig, MonoMarshalSpec** mspecs) { @@ -2253,7 +2334,7 @@ emit_thunk_invoke_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *method, Mono MonoImage *image = get_method_image (method); MonoMethodSignature *sig = mono_method_signature_internal (method); int param_count = sig->param_count + sig->hasthis + 1; - int pos_leave, coop_gc_var = 0; + int pos_leave; MonoExceptionClause *clause; MonoType *object_type = mono_get_object_type (); #if defined (TARGET_WASM) @@ -2266,6 +2347,10 @@ emit_thunk_invoke_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *method, Mono #else const gboolean do_blocking_transition = TRUE; #endif + GCUnsafeTransitionBuilder gc_unsafe_builder = {0,}; + + if (do_blocking_transition) + gc_unsafe_transition_builder_init (&gc_unsafe_builder, mb, TRUE); /* local 0 (temp for exception object) */ mono_mb_add_local (mb, object_type); @@ -2275,8 +2360,7 @@ emit_thunk_invoke_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *method, Mono mono_mb_add_local (mb, sig->ret); if (do_blocking_transition) { - /* local 4, the local to be used when calling the suspend funcs */ - coop_gc_var = mono_mb_add_local (mb, mono_get_int_type ()); + gc_unsafe_transition_builder_add_vars (&gc_unsafe_builder); } /* clear exception arg */ @@ -2285,10 +2369,7 @@ emit_thunk_invoke_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *method, Mono mono_mb_emit_byte (mb, CEE_STIND_REF); if (do_blocking_transition) { - mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX); - mono_mb_emit_byte (mb, CEE_MONO_GET_SP); - mono_mb_emit_icall (mb, mono_threads_enter_gc_unsafe_region_unbalanced); - mono_mb_emit_stloc (mb, coop_gc_var); + gc_unsafe_transition_builder_emit_enter (&gc_unsafe_builder); } /* try */ @@ -2361,10 +2442,9 @@ emit_thunk_invoke_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *method, Mono } if (do_blocking_transition) { - mono_mb_emit_ldloc (mb, coop_gc_var); - mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX); - mono_mb_emit_byte (mb, CEE_MONO_GET_SP); - mono_mb_emit_icall (mb, mono_threads_exit_gc_unsafe_region_unbalanced); + gc_unsafe_transition_builder_emit_exit (&gc_unsafe_builder); + + gc_unsafe_transition_builder_cleanup (&gc_unsafe_builder); } mono_mb_emit_byte (mb, CEE_RET); @@ -2414,8 +2494,9 @@ static void emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_sig, MonoMarshalSpec **mspecs, EmitMarshalContext* m, MonoMethod *method, MonoGCHandle target_handle, MonoError *error) { MonoMethodSignature *sig, *csig; - int i, *tmp_locals, orig_domain, attach_cookie; + int i, *tmp_locals; gboolean closed = FALSE; + GCUnsafeTransitionBuilder gc_unsafe_builder = {0,}; sig = m->sig; csig = m->csig; @@ -2453,8 +2534,8 @@ emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_s if (MONO_TYPE_ISSTRUCT (sig->ret)) m->vtaddr_var = mono_mb_add_local (mb, int_type); - orig_domain = mono_mb_add_local (mb, int_type); - attach_cookie = mono_mb_add_local (mb, int_type); + gc_unsafe_transition_builder_init (&gc_unsafe_builder, mb, TRUE); + gc_unsafe_transition_builder_add_vars (&gc_unsafe_builder); /* * // does (STARTING|RUNNING|BLOCKING) -> RUNNING + set/switch domain @@ -2472,21 +2553,7 @@ emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_s mono_mb_emit_icon (mb, 0); mono_mb_emit_stloc (mb, 2); - /* orig_domain = mono_threads_attach_coop (domain, &attach_cookie); */ - mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX); - mono_mb_emit_byte (mb, CEE_MONO_LDDOMAIN); - mono_mb_emit_ldloc_addr (mb, attach_cookie); - /* - * This icall is special cased in the JIT so it works in native-to-managed wrappers in unattached threads. - * Keep this in sync with the CEE_JIT_ICALL code in the JIT. - * - * Special cased in interpreter, keep in sync. - */ - mono_mb_emit_icall (mb, mono_threads_attach_coop); - mono_mb_emit_stloc (mb, orig_domain); - - /* */ - emit_thread_interrupt_checkpoint (mb); + gc_unsafe_transition_builder_emit_enter(&gc_unsafe_builder); /* we first do all conversions */ tmp_locals = g_newa (int, sig->param_count); @@ -2639,11 +2706,9 @@ emit_managed_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke_s } } - /* mono_threads_detach_coop (orig_domain, &attach_cookie); */ - mono_mb_emit_ldloc (mb, orig_domain); - mono_mb_emit_ldloc_addr (mb, attach_cookie); - /* Special cased in interpreter, keep in sync */ - mono_mb_emit_icall (mb, mono_threads_detach_coop); + gc_unsafe_transition_builder_emit_exit (&gc_unsafe_builder); + + gc_unsafe_transition_builder_cleanup (&gc_unsafe_builder); /* return ret; */ if (m->retobj_var) { From 779b3ff9c75e59a152c897211ca983d0baf5ec26 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 13 Feb 2023 09:34:42 -0500 Subject: [PATCH 41/43] fix whitespace --- .../System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index f9801fa36d16b0..febe5e5b9d06ce 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -46,7 +46,6 @@ public static void TryEnter(object obj, ref bool lockTaken) { if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); - ReliableEnterTimeout(obj, 0, ref lockTaken); } @@ -62,7 +61,6 @@ public static void TryEnter(object obj, int millisecondsTimeout, ref bool lockTa { if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); - ReliableEnterTimeout(obj, millisecondsTimeout, ref lockTaken); } From 71a9422b2c69779ad372cabe9349c6491704b7ae Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 13 Feb 2023 09:38:52 -0500 Subject: [PATCH 42/43] more whitespace --- .../System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs index febe5e5b9d06ce..96424ec2bffa9a 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs @@ -46,13 +46,13 @@ public static void TryEnter(object obj, ref bool lockTaken) { if (lockTaken) throw new ArgumentException(SR.Argument_MustBeFalse, nameof(lockTaken)); + ReliableEnterTimeout(obj, 0, ref lockTaken); } public static bool TryEnter(object obj, int millisecondsTimeout) { bool lockTaken = false; - TryEnter(obj, millisecondsTimeout, ref lockTaken); return lockTaken; } From ee396824021768891333fcfc20f0cc5b635978a3 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 14 Feb 2023 10:07:21 -0500 Subject: [PATCH 43/43] [jiterp] Allow null obj in hashcode intrinsics The underlying C functions are null-friendly, and the managed intrinsics are allowed to C null inputs. The assert is unnecessary. --- src/mono/mono/mini/interp/jiterpreter.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/mono/mono/mini/interp/jiterpreter.c b/src/mono/mono/mini/interp/jiterpreter.c index c6c40d45482872..cef0a335eff476 100644 --- a/src/mono/mono/mini/interp/jiterpreter.c +++ b/src/mono/mono/mini/interp/jiterpreter.c @@ -1110,7 +1110,6 @@ EMSCRIPTEN_KEEPALIVE int mono_jiterp_get_hashcode (MonoObject ** ppObj) { MonoObject *obj = *ppObj; - g_assert (obj); return mono_object_hash_internal (obj); } @@ -1118,7 +1117,6 @@ EMSCRIPTEN_KEEPALIVE int mono_jiterp_try_get_hashcode (MonoObject ** ppObj) { MonoObject *obj = *ppObj; - g_assert (obj); return mono_object_try_get_hash_internal (obj); }