From 5b3126e430e3319950482fdb3f2b7781316c028d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Sun, 3 Mar 2024 02:44:22 +0100 Subject: [PATCH 01/20] Make delegates immutable --- .../src/System/Delegate.CoreCLR.cs | 112 ++++++++++-------- .../src/System/MulticastDelegate.CoreCLR.cs | 37 +++--- src/coreclr/vm/amd64/asmconstants.h | 10 +- src/coreclr/vm/arm/asmconstants.h | 2 +- src/coreclr/vm/arm64/asmconstants.h | 2 +- src/coreclr/vm/comdelegate.cpp | 4 +- src/coreclr/vm/corelib.h | 2 +- src/coreclr/vm/i386/asmconstants.h | 8 +- src/coreclr/vm/loongarch64/asmconstants.h | 2 +- src/coreclr/vm/object.cpp | 11 ++ src/coreclr/vm/object.h | 5 +- src/coreclr/vm/riscv64/asmconstants.h | 2 +- 12 files changed, 111 insertions(+), 86 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index 1168784b30563..cee476ce61ae8 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -14,13 +14,14 @@ namespace System [ComVisible(true)] public abstract partial class Delegate : ICloneable, ISerializable { + // Caches MethodInfos, added either after first request or assigned from a DynamicMethod + // For open delegates to collectible types, this may contain a LoaderAllocator object + // that prevents the type from being unloaded. + internal static readonly ConditionalWeakTable s_methodCache = new(); + // _target is the object we will invoke on internal object? _target; // Initialized by VM as needed; null if static delegate - // MethodBase, either cached after first request or assigned from a DynamicMethod - // For open delegates to collectible types, this may be a LoaderAllocator object - internal object? _methodBase; // Initialized by VM as needed - // _methodPtr is a pointer to the method we will invoke // It could be a small thunk if this is a static or UM call internal IntPtr _methodPtr; @@ -130,10 +131,10 @@ public override bool Equals([NotNullWhen(true)] object? obj) // method ptrs don't match, go down long path // - if (_methodBase == null || d._methodBase == null || !(_methodBase is MethodInfo) || !(d._methodBase is MethodInfo)) - return InternalEqualMethodHandles(this, d); - else - return _methodBase.Equals(d._methodBase); + if (s_methodCache.TryGetValue(this, out object? thisCache) && thisCache is MethodInfo thisMethod && + s_methodCache.TryGetValue(d, out object? otherCache) && otherCache is MethodInfo otherMethod) + return thisMethod.Equals(otherMethod); + return InternalEqualMethodHandles(this, d); } public override int GetHashCode() @@ -156,57 +157,70 @@ public override int GetHashCode() protected virtual MethodInfo GetMethodImpl() { - if ((_methodBase == null) || !(_methodBase is MethodInfo)) + if (s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) + { + return methodInfo; + } + + return GetMethodImplUncached(); + } + + private MethodInfo GetMethodImplUncached() + { + IRuntimeMethodInfo method = FindMethodHandle(); + RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method); + // need a proper declaring type instance method on a generic type + if (declaringType.IsGenericType) { - IRuntimeMethodInfo method = FindMethodHandle(); - RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method); - // need a proper declaring type instance method on a generic type - if (declaringType.IsGenericType) + bool isStatic = (RuntimeMethodHandle.GetAttributes(method) & MethodAttributes.Static) != (MethodAttributes)0; + if (!isStatic) { - bool isStatic = (RuntimeMethodHandle.GetAttributes(method) & MethodAttributes.Static) != (MethodAttributes)0; - if (!isStatic) + if (_methodPtrAux == IntPtr.Zero) { - if (_methodPtrAux == IntPtr.Zero) + // The target may be of a derived type that doesn't have visibility onto the + // target method. We don't want to call RuntimeType.GetMethodBase below with that + // or reflection can end up generating a MethodInfo where the ReflectedType cannot + // see the MethodInfo itself and that breaks an important invariant. But the + // target type could include important generic type information we need in order + // to work out what the exact instantiation of the method's declaring type is. So + // we'll walk up the inheritance chain (which will yield exactly instantiated + // types at each step) until we find the declaring type. Since the declaring type + // we get from the method is probably shared and those in the hierarchy we're + // walking won't be we compare using the generic type definition forms instead. + Type? currentType = _target!.GetType(); + Type targetType = declaringType.GetGenericTypeDefinition(); + while (currentType != null) { - // The target may be of a derived type that doesn't have visibility onto the - // target method. We don't want to call RuntimeType.GetMethodBase below with that - // or reflection can end up generating a MethodInfo where the ReflectedType cannot - // see the MethodInfo itself and that breaks an important invariant. But the - // target type could include important generic type information we need in order - // to work out what the exact instantiation of the method's declaring type is. So - // we'll walk up the inheritance chain (which will yield exactly instantiated - // types at each step) until we find the declaring type. Since the declaring type - // we get from the method is probably shared and those in the hierarchy we're - // walking won't be we compare using the generic type definition forms instead. - Type? currentType = _target!.GetType(); - Type targetType = declaringType.GetGenericTypeDefinition(); - while (currentType != null) + if (currentType.IsGenericType && + currentType.GetGenericTypeDefinition() == targetType) { - if (currentType.IsGenericType && - currentType.GetGenericTypeDefinition() == targetType) - { - declaringType = currentType as RuntimeType; - break; - } - currentType = currentType.BaseType; + declaringType = currentType as RuntimeType; + break; } - - // RCWs don't need to be "strongly-typed" in which case we don't find a base type - // that matches the declaring type of the method. This is fine because interop needs - // to work with exact methods anyway so declaringType is never shared at this point. - Debug.Assert(currentType != null || _target.GetType().IsCOMObject, "The class hierarchy should declare the method"); - } - else - { - // it's an open one, need to fetch the first arg of the instantiation - MethodInfo invoke = this.GetType().GetMethod("Invoke")!; - declaringType = (RuntimeType)invoke.GetParametersAsSpan()[0].ParameterType; + currentType = currentType.BaseType; } + + // RCWs don't need to be "strongly-typed" in which case we don't find a base type + // that matches the declaring type of the method. This is fine because interop needs + // to work with exact methods anyway so declaringType is never shared at this point. + Debug.Assert(currentType != null || _target.GetType().IsCOMObject, "The class hierarchy should declare the method"); + } + else + { + // it's an open one, need to fetch the first arg of the instantiation + MethodInfo invoke = GetType().GetMethod("Invoke")!; + declaringType = (RuntimeType)invoke.GetParametersAsSpan()[0].ParameterType; } } - _methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!; } - return (MethodInfo)_methodBase; + MethodInfo methodInfo = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!; + SetCachedMethod(methodInfo); + return methodInfo; + } + + internal void SetCachedMethod(object? value) + { + s_methodCache.AddOrUpdate(this, value); } public object? Target => GetTarget(); diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs index ab63d4cf83b74..fd85dbdf64614 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs @@ -200,10 +200,10 @@ internal void StoreDynamicMethod(MethodInfo dynamicMethod) Debug.Assert(!IsUnmanagedFunctionPtr(), "dynamic method and unmanaged fntptr delegate combined"); // must be a secure/wrapper one, unwrap and save MulticastDelegate d = ((MulticastDelegate?)_invocationList)!; - d._methodBase = dynamicMethod; + d.SetCachedMethod(dynamicMethod); } else - _methodBase = dynamicMethod; + SetCachedMethod(dynamicMethod); } // This method will combine this delegate with the passed delegate @@ -515,21 +515,24 @@ protected override MethodInfo GetMethodImpl() { // we handle unmanaged function pointers here because the generic ones (used for WinRT) would otherwise // be treated as open delegates by the base implementation, resulting in failure to get the MethodInfo - if ((_methodBase == null) || !(_methodBase is MethodInfo)) + if (s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) { - IRuntimeMethodInfo method = FindMethodHandle(); - RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method); + return methodInfo; + } - // need a proper declaring type instance method on a generic type - if (declaringType.IsGenericType) - { - // we are returning the 'Invoke' method of this delegate so use this.GetType() for the exact type - RuntimeType reflectedType = (RuntimeType)GetType(); - declaringType = reflectedType; - } - _methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!; + IRuntimeMethodInfo method = FindMethodHandle(); + RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method); + + // need a proper declaring type instance method on a generic type + if (declaringType.IsGenericType) + { + // we are returning the 'Invoke' method of this delegate so use this.GetType() for the exact type + RuntimeType reflectedType = (RuntimeType)GetType(); + declaringType = reflectedType; } - return (MethodInfo)_methodBase; + methodInfo = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!; + SetCachedMethod(methodInfo); + return methodInfo; } // Otherwise, must be an inner delegate of a wrapper delegate of an open virtual method. In that case, call base implementation @@ -595,7 +598,7 @@ private void CtorCollectibleClosedStatic(object target, IntPtr methodPtr, IntPtr { this._target = target; this._methodPtr = methodPtr; - this._methodBase = GCHandle.InternalGet(gchandle); + this.SetCachedMethod(GCHandle.InternalGet(gchandle)); } [DebuggerNonUserCode] @@ -605,7 +608,7 @@ private void CtorCollectibleOpened(object target, IntPtr methodPtr, IntPtr shuff this._target = this; this._methodPtr = shuffleThunk; this._methodPtrAux = methodPtr; - this._methodBase = GCHandle.InternalGet(gchandle); + this.SetCachedMethod(GCHandle.InternalGet(gchandle)); } [DebuggerNonUserCode] @@ -614,7 +617,7 @@ private void CtorCollectibleVirtualDispatch(object target, IntPtr methodPtr, Int { this._target = this; this._methodPtr = shuffleThunk; - this._methodBase = GCHandle.InternalGet(gchandle); + this.SetCachedMethod(GCHandle.InternalGet(gchandle)); this.InitializeVirtualCallStub(methodPtr); } #pragma warning restore IDE0060 diff --git a/src/coreclr/vm/amd64/asmconstants.h b/src/coreclr/vm/amd64/asmconstants.h index e12f3e1eafd26..277fe6b23816b 100644 --- a/src/coreclr/vm/amd64/asmconstants.h +++ b/src/coreclr/vm/amd64/asmconstants.h @@ -138,7 +138,7 @@ ASMCONSTANTS_C_ASSERT(OFFSETOF__ThreadExceptionState__m_pCurrentTracker -#define OFFSETOF__DelegateObject___methodPtr 0x18 +#define OFFSETOF__DelegateObject___methodPtr 0x10 ASMCONSTANT_OFFSETOF_ASSERT(DelegateObject, _methodPtr); #define OFFSETOF__DelegateObject___target 0x08 @@ -680,13 +680,13 @@ template class FindCompileTimeConstant { private: - FindCompileTimeConstant(); + FindCompileTimeConstant(); }; void BogusFunction() { - // Sample usage to generate the error - FindCompileTimeConstant bogus_variable; - FindCompileTimeConstant bogus_variable2; + // Sample usage to generate the error + FindCompileTimeConstant bogus_variable; + FindCompileTimeConstant bogus_variable2; } #endif // defined(__cplusplus) && defined(USE_COMPILE_TIME_CONSTANT_FINDER) diff --git a/src/coreclr/vm/arm/asmconstants.h b/src/coreclr/vm/arm/asmconstants.h index 16931168e3ce0..76b583c8e2893 100644 --- a/src/coreclr/vm/arm/asmconstants.h +++ b/src/coreclr/vm/arm/asmconstants.h @@ -64,7 +64,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureSp == offsetof(LazyMachState, capture #define LazyMachState_captureIp (LazyMachState_captureSp+4) ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, captureIp)) -#define DelegateObject___methodPtr 0x0c +#define DelegateObject___methodPtr 0x08 ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x04 diff --git a/src/coreclr/vm/arm64/asmconstants.h b/src/coreclr/vm/arm64/asmconstants.h index cc19728d9e29e..5b67217d47d4a 100644 --- a/src/coreclr/vm/arm64/asmconstants.h +++ b/src/coreclr/vm/arm64/asmconstants.h @@ -109,7 +109,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, capture #define VASigCookie__pNDirectILStub 0x8 ASMCONSTANTS_C_ASSERT(VASigCookie__pNDirectILStub == offsetof(VASigCookie, pNDirectILStub)) -#define DelegateObject___methodPtr 0x18 +#define DelegateObject___methodPtr 0x10 ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x08 diff --git a/src/coreclr/vm/comdelegate.cpp b/src/coreclr/vm/comdelegate.cpp index ef4021039a66b..6597486ca8e16 100644 --- a/src/coreclr/vm/comdelegate.cpp +++ b/src/coreclr/vm/comdelegate.cpp @@ -2808,8 +2808,8 @@ MethodDesc* COMDelegate::GetDelegateCtor(TypeHandle delegateType, MethodDesc *pT LoaderAllocator *pTargetMethodLoaderAllocator = pTargetMethod->GetLoaderAllocator(); BOOL isCollectible = pTargetMethodLoaderAllocator->IsCollectible(); // A method that may be instantiated over a collectible type, and is static will require a delegate - // that has the _methodBase field filled in with the LoaderAllocator of the collectible assembly - // associated with the instantiation. + // that has the LoaderAllocator of the collectible assembly associated with the instantiation + // stored in the MethodInfo cache. BOOL fMaybeCollectibleAndStatic = FALSE; // Do not allow static methods with [UnmanagedCallersOnlyAttribute] to be a delegate target. diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index c52c58954165a..12b5b6b1d1ac2 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -249,7 +249,6 @@ DEFINE_METHOD(DECIMAL, CURRENCY_CTOR, .ctor, DEFINE_CLASS_U(System, Delegate, NoClass) DEFINE_FIELD_U(_target, DelegateObject, _target) -DEFINE_FIELD_U(_methodBase, DelegateObject, _methodBase) DEFINE_FIELD_U(_methodPtr, DelegateObject, _methodPtr) DEFINE_FIELD_U(_methodPtrAux, DelegateObject, _methodPtrAux) DEFINE_CLASS(DELEGATE, System, Delegate) @@ -258,6 +257,7 @@ DEFINE_FIELD(DELEGATE, METHOD_PTR, _methodPtr) DEFINE_FIELD(DELEGATE, METHOD_PTR_AUX, _methodPtrAux) DEFINE_METHOD(DELEGATE, CONSTRUCT_DELEGATE, DelegateConstruct, IM_Obj_IntPtr_RetVoid) DEFINE_METHOD(DELEGATE, GET_INVOKE_METHOD, GetInvokeMethod, IM_RetIntPtr) +DEFINE_METHOD(DELEGATE, SET_CACHED_METHOD, SetCachedMethod, IM_Obj_RetVoid) DEFINE_CLASS(INT128, System, Int128) DEFINE_CLASS(UINT128, System, UInt128) diff --git a/src/coreclr/vm/i386/asmconstants.h b/src/coreclr/vm/i386/asmconstants.h index 7de14a6c06318..2437efcb72d58 100644 --- a/src/coreclr/vm/i386/asmconstants.h +++ b/src/coreclr/vm/i386/asmconstants.h @@ -301,14 +301,12 @@ ASMCONSTANTS_C_ASSERT(InlinedCallFrame__m_pCalleeSavedFP == offsetof(InlinedCall #ifdef FEATURE_STUBS_AS_IL // DelegateObject from src/vm/object.h #define DelegateObject___target 0x04 // offset 0 is m_pMethTab of base class Object -#define DelegateObject___methodBase 0x08 -#define DelegateObject___methodPtr 0x0c -#define DelegateObject___methodPtrAux 0x10 +#define DelegateObject___methodPtr 0x08 +#define DelegateObject___methodPtrAux 0x0c #define DelegateObject___invocationList 0x14 -#define DelegateObject___invocationCount 0x18 +#define DelegateObject___invocationCount 0x14 ASMCONSTANTS_C_ASSERT(DelegateObject___target == offsetof(DelegateObject, _target)); -ASMCONSTANTS_C_ASSERT(DelegateObject___methodBase == offsetof(DelegateObject, _methodBase)); ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtrAux == offsetof(DelegateObject, _methodPtrAux)); ASMCONSTANTS_C_ASSERT(DelegateObject___invocationList == offsetof(DelegateObject, _invocationList)); diff --git a/src/coreclr/vm/loongarch64/asmconstants.h b/src/coreclr/vm/loongarch64/asmconstants.h index e12d0040a74d0..2d47ceea2f267 100644 --- a/src/coreclr/vm/loongarch64/asmconstants.h +++ b/src/coreclr/vm/loongarch64/asmconstants.h @@ -134,7 +134,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, capture #define VASigCookie__pNDirectILStub 0x8 ASMCONSTANTS_C_ASSERT(VASigCookie__pNDirectILStub == offsetof(VASigCookie, pNDirectILStub)) -#define DelegateObject___methodPtr 0x18 +#define DelegateObject___methodPtr 0x10 ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x08 diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 213a9cde5925b..be2d13c1e7925 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1394,6 +1394,17 @@ void __fastcall ZeroMemoryInGCHeap(void* mem, size_t size) *memBytes++ = 0; } +void DelegateObject::SetMethodBase(OBJECTREF newMethodBase) +{ + STANDARD_VM_CONTRACT; + GCX_COOP(); + PREPARE_NONVIRTUAL_CALLSITE(METHOD__DELEGATE__SET_CACHED_METHOD); + DECLARE_ARGHOLDER_ARRAY(args, 2); + args[ARGNUM_0] = PTR_TO_ARGHOLDER(this); + args[ARGNUM_1] = OBJECTREF_TO_ARGHOLDER(newMethodBase); + CALL_MANAGED_METHOD_NORET(args); +} + void StackTraceArray::Append(StackTraceElement const * begin, StackTraceElement const * end) { CONTRACTL diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 915e45deca063..5617cf84e9a01 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -2000,7 +2000,7 @@ class DelegateObject : public Object void SetInvocationCount(INT_PTR invocationCount) { LIMITED_METHOD_CONTRACT; _invocationCount = invocationCount; } static int GetOffsetOfInvocationCount() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _invocationCount); } - void SetMethodBase(OBJECTREF newMethodBase) { LIMITED_METHOD_CONTRACT; SetObjectReference((OBJECTREF*)&_methodBase, newMethodBase); } + void SetMethodBase(OBJECTREF newMethodBase); // README: // If you modify the order of these fields, make sure to update the definition in @@ -2008,7 +2008,6 @@ class DelegateObject : public Object private: // System.Delegate OBJECTREF _target; - OBJECTREF _methodBase; PCODE _methodPtr; PCODE _methodPtrAux; // System.MulticastDelegate @@ -2017,7 +2016,7 @@ class DelegateObject : public Object }; #define OFFSETOF__DelegateObject__target OBJECT_SIZE /* m_pMethTab */ -#define OFFSETOF__DelegateObject__methodPtr (OFFSETOF__DelegateObject__target + TARGET_POINTER_SIZE /* _target */ + TARGET_POINTER_SIZE /* _methodBase */) +#define OFFSETOF__DelegateObject__methodPtr (OFFSETOF__DelegateObject__target + TARGET_POINTER_SIZE /* _target */) #define OFFSETOF__DelegateObject__methodPtrAux (OFFSETOF__DelegateObject__methodPtr + TARGET_POINTER_SIZE /* _methodPtr */) #ifdef USE_CHECKED_OBJECTREFS diff --git a/src/coreclr/vm/riscv64/asmconstants.h b/src/coreclr/vm/riscv64/asmconstants.h index 71095e3cffc99..57f9a63fbbd17 100644 --- a/src/coreclr/vm/riscv64/asmconstants.h +++ b/src/coreclr/vm/riscv64/asmconstants.h @@ -129,7 +129,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, capture #define VASigCookie__pNDirectILStub 0x8 ASMCONSTANTS_C_ASSERT(VASigCookie__pNDirectILStub == offsetof(VASigCookie, pNDirectILStub)) -#define DelegateObject___methodPtr 0x18 +#define DelegateObject___methodPtr 0x10 ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x08 From 64baa1d37fb26ad0e216ef7a9fb6f1bf17892cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Sun, 3 Mar 2024 02:53:35 +0100 Subject: [PATCH 02/20] Add a comment --- .../System.Private.CoreLib/src/System/Delegate.CoreCLR.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index cee476ce61ae8..f7179f7005394 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -218,6 +218,7 @@ private MethodInfo GetMethodImplUncached() return methodInfo; } + // this is called by the VM in addition to calls from managed code internal void SetCachedMethod(object? value) { s_methodCache.AddOrUpdate(this, value); From a489288ec2e78e4969a65216e27799f706fc0c3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Sun, 3 Mar 2024 03:00:23 +0100 Subject: [PATCH 03/20] Optimize cache checks --- .../src/System/Delegate.CoreCLR.cs | 2 +- .../src/System/MulticastDelegate.CoreCLR.cs | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index f7179f7005394..e6472a947336e 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -165,7 +165,7 @@ protected virtual MethodInfo GetMethodImpl() return GetMethodImplUncached(); } - private MethodInfo GetMethodImplUncached() + internal MethodInfo GetMethodImplUncached() { IRuntimeMethodInfo method = FindMethodHandle(); RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method); diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs index fd85dbdf64614..caec944abe8a4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs @@ -495,6 +495,16 @@ public sealed override int GetHashCode() } protected override MethodInfo GetMethodImpl() + { + if (s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) + { + return methodInfo; + } + + return GetMulticastMethodImplUncached(); + } + + internal MethodInfo GetMulticastMethodImplUncached() { if (_invocationCount != 0 && _invocationList != null) { @@ -515,10 +525,6 @@ protected override MethodInfo GetMethodImpl() { // we handle unmanaged function pointers here because the generic ones (used for WinRT) would otherwise // be treated as open delegates by the base implementation, resulting in failure to get the MethodInfo - if (s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) - { - return methodInfo; - } IRuntimeMethodInfo method = FindMethodHandle(); RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method); @@ -530,13 +536,13 @@ protected override MethodInfo GetMethodImpl() RuntimeType reflectedType = (RuntimeType)GetType(); declaringType = reflectedType; } - methodInfo = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!; + MethodInfo methodInfo = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!; SetCachedMethod(methodInfo); return methodInfo; } // Otherwise, must be an inner delegate of a wrapper delegate of an open virtual method. In that case, call base implementation - return base.GetMethodImpl(); + return GetMethodImplUncached(); } // this should help inlining From bc132e438b38b28e99146a5c1a9369788ce18026 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Sun, 3 Mar 2024 23:07:46 +0100 Subject: [PATCH 04/20] Fix x86 --- .../src/System/Delegate.CoreCLR.cs | 21 +++++++++++-------- .../src/System/MulticastDelegate.CoreCLR.cs | 2 +- src/coreclr/vm/i386/asmconstants.h | 2 +- .../src/System/Delegate.cs | 6 +++--- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index e6472a947336e..da6b41b3306da 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -14,11 +14,6 @@ namespace System [ComVisible(true)] public abstract partial class Delegate : ICloneable, ISerializable { - // Caches MethodInfos, added either after first request or assigned from a DynamicMethod - // For open delegates to collectible types, this may contain a LoaderAllocator object - // that prevents the type from being unloaded. - internal static readonly ConditionalWeakTable s_methodCache = new(); - // _target is the object we will invoke on internal object? _target; // Initialized by VM as needed; null if static delegate @@ -131,8 +126,8 @@ public override bool Equals([NotNullWhen(true)] object? obj) // method ptrs don't match, go down long path // - if (s_methodCache.TryGetValue(this, out object? thisCache) && thisCache is MethodInfo thisMethod && - s_methodCache.TryGetValue(d, out object? otherCache) && otherCache is MethodInfo otherMethod) + if (Cache.s_methodCache.TryGetValue(this, out object? thisCache) && thisCache is MethodInfo thisMethod && + Cache.s_methodCache.TryGetValue(d, out object? otherCache) && otherCache is MethodInfo otherMethod) return thisMethod.Equals(otherMethod); return InternalEqualMethodHandles(this, d); } @@ -157,7 +152,7 @@ public override int GetHashCode() protected virtual MethodInfo GetMethodImpl() { - if (s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) + if (Cache.s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) { return methodInfo; } @@ -221,7 +216,7 @@ internal MethodInfo GetMethodImplUncached() // this is called by the VM in addition to calls from managed code internal void SetCachedMethod(object? value) { - s_methodCache.AddOrUpdate(this, value); + Cache.s_methodCache.AddOrUpdate(this, value); } public object? Target => GetTarget(); @@ -534,6 +529,14 @@ internal void InitializeVirtualCallStub(IntPtr methodPtr) { return (_methodPtrAux == IntPtr.Zero) ? _target : null; } + + // Caches MethodInfos, added either after first request or assigned from a DynamicMethod + // For open delegates to collectible types, this may contain a LoaderAllocator object + // that prevents the type from being unloaded. + internal static class Cache + { + public static readonly ConditionalWeakTable s_methodCache = new(); + } } // These flags effect the way BindToMethodInfo and BindToMethodName are allowed to bind a delegate to a target method. Their diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs index caec944abe8a4..c84d383b3e0c2 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs @@ -496,7 +496,7 @@ public sealed override int GetHashCode() protected override MethodInfo GetMethodImpl() { - if (s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) + if (Cache.s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) { return methodInfo; } diff --git a/src/coreclr/vm/i386/asmconstants.h b/src/coreclr/vm/i386/asmconstants.h index 2437efcb72d58..1ab9406b8ec6c 100644 --- a/src/coreclr/vm/i386/asmconstants.h +++ b/src/coreclr/vm/i386/asmconstants.h @@ -303,7 +303,7 @@ ASMCONSTANTS_C_ASSERT(InlinedCallFrame__m_pCalleeSavedFP == offsetof(InlinedCall #define DelegateObject___target 0x04 // offset 0 is m_pMethTab of base class Object #define DelegateObject___methodPtr 0x08 #define DelegateObject___methodPtrAux 0x0c -#define DelegateObject___invocationList 0x14 +#define DelegateObject___invocationList 0x10 #define DelegateObject___invocationCount 0x14 ASMCONSTANTS_C_ASSERT(DelegateObject___target == offsetof(DelegateObject, _target)); diff --git a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs index d24059770d5f8..6660c3976df00 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs @@ -74,8 +74,8 @@ public abstract partial class Delegate : ICloneable, ISerializable /// The order of the delegates returned by the enumerator is the same order in which the current delegate invokes the methods that those delegates represent. /// The method returns an empty enumerator for null delegate. /// - public static System.Delegate.InvocationListEnumerator EnumerateInvocationList(TDelegate? d) where TDelegate : System.Delegate - => new InvocationListEnumerator(Unsafe.As(d)); + public static Delegate.InvocationListEnumerator EnumerateInvocationList(TDelegate? d) where TDelegate : System.Delegate + => new Delegate.InvocationListEnumerator(Unsafe.As(d)); /// /// Provides an enumerator for the invocation list of a delegate. @@ -119,7 +119,7 @@ public bool MoveNext() /// Implement IEnumerable.GetEnumerator() to return 'this' as the IEnumerator /// [EditorBrowsable(EditorBrowsableState.Never)] // Only here to make foreach work - public System.Delegate.InvocationListEnumerator GetEnumerator() => this; + public Delegate.InvocationListEnumerator GetEnumerator() => this; } public object? DynamicInvoke(params object?[]? args) From 2dce6a845646ff98a790e61762e2b26961b5cdbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Mon, 4 Mar 2024 01:06:58 +0100 Subject: [PATCH 05/20] Fix R2R --- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index ad83b1eb42a5d..4cd6f5abae7b2 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -456,7 +456,7 @@ unsafe partial class CorInfoImpl { private const CORINFO_RUNTIME_ABI TargetABI = CORINFO_RUNTIME_ABI.CORINFO_CORECLR_ABI; - private uint OffsetOfDelegateFirstTarget => (uint)(3 * PointerSize); // Delegate::m_functionPointer + private uint OffsetOfDelegateFirstTarget => (uint)(2 * PointerSize); // Delegate::m_functionPointer private readonly ReadyToRunCodegenCompilation _compilation; private MethodWithGCInfo _methodCodeNode; @@ -2114,7 +2114,7 @@ private void ceeInfoGetCallInfo( // of shared generic code calling a shared generic implementation method, which should be rare. // // An alternative design would be to add a new generic dictionary entry kind to hold the MethodDesc - // of the constrained target instead, and use that in some circumstances; however, implementation of + // of the constrained target instead, and use that in some circumstances; however, implementation of // that design requires refactoring variuos parts of the JIT interface as well as // TryResolveConstraintMethodApprox. In particular we would need to be abled to embed a constrained lookup // via EmbedGenericHandle, as well as decide in TryResolveConstraintMethodApprox if the call can be made From 00fcd02e187515bbcec2b7c74adb4f4842169d52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 4 Mar 2024 16:30:58 +0100 Subject: [PATCH 06/20] Update methodcontext.cpp --- src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index c72b8b1eec1f6..efd869681442f 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -4370,7 +4370,7 @@ void MethodContext::repGetEEInfo(CORINFO_EE_INFO* pEEInfoOut) pEEInfoOut->offsetOfThreadFrame = (unsigned)0x10; pEEInfoOut->offsetOfGCState = (unsigned)0xc; pEEInfoOut->offsetOfDelegateInstance = (unsigned)0x8; - pEEInfoOut->offsetOfDelegateFirstTarget = (unsigned)0x18; + pEEInfoOut->offsetOfDelegateFirstTarget = (unsigned)0x10; pEEInfoOut->offsetOfWrapperDelegateIndirectCell = (unsigned)0x40; pEEInfoOut->sizeOfReversePInvokeFrame = (unsigned)0x8; pEEInfoOut->osPageSize = (size_t)0x1000; From 7941123c0d8d6121e7ec8f777b127f76382ba8ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 4 Mar 2024 16:37:03 +0100 Subject: [PATCH 07/20] Fix contract --- src/coreclr/vm/object.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index be2d13c1e7925..4963c6ea3b130 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1396,7 +1396,14 @@ void __fastcall ZeroMemoryInGCHeap(void* mem, size_t size) void DelegateObject::SetMethodBase(OBJECTREF newMethodBase) { - STANDARD_VM_CONTRACT; + CONTRACTL + { + NOTHROW; + GC_TRIGGERS; + MODE_ANY; + } + CONTRACTL_END; + GCX_COOP(); PREPARE_NONVIRTUAL_CALLSITE(METHOD__DELEGATE__SET_CACHED_METHOD); DECLARE_ARGHOLDER_ARRAY(args, 2); From 4de27544979df7655149b7202ce1f1f906447154 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 4 Mar 2024 16:40:11 +0100 Subject: [PATCH 08/20] Update src/coreclr/vm/object.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/object.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 4963c6ea3b130..a5c7cbc82bac2 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1400,11 +1400,10 @@ void DelegateObject::SetMethodBase(OBJECTREF newMethodBase) { NOTHROW; GC_TRIGGERS; - MODE_ANY; + MODE_COOPERATIVE; } CONTRACTL_END; - GCX_COOP(); PREPARE_NONVIRTUAL_CALLSITE(METHOD__DELEGATE__SET_CACHED_METHOD); DECLARE_ARGHOLDER_ARRAY(args, 2); args[ARGNUM_0] = PTR_TO_ARGHOLDER(this); From b9f7f14000fb2307eaa6eb373ce4f584a4b9b475 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Mon, 4 Mar 2024 17:01:21 +0100 Subject: [PATCH 09/20] Swap fields --- .../src/System/Delegate.CoreCLR.cs | 8 ++++---- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 2 +- .../superpmi/superpmi-shared/methodcontext.cpp | 8 ++++---- src/coreclr/vm/amd64/asmconstants.h | 2 +- src/coreclr/vm/arm/asmconstants.h | 2 +- src/coreclr/vm/arm64/asmconstants.h | 2 +- src/coreclr/vm/corelib.h | 14 +++++++------- src/coreclr/vm/i386/asmconstants.h | 4 ++-- src/coreclr/vm/loongarch64/asmconstants.h | 2 +- src/coreclr/vm/object.h | 14 +++++++------- src/coreclr/vm/riscv64/asmconstants.h | 2 +- 11 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index da6b41b3306da..76912df5ff93b 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -17,16 +17,16 @@ public abstract partial class Delegate : ICloneable, ISerializable // _target is the object we will invoke on internal object? _target; // Initialized by VM as needed; null if static delegate - // _methodPtr is a pointer to the method we will invoke - // It could be a small thunk if this is a static or UM call - internal IntPtr _methodPtr; - // In the case of a static method passed to a delegate, this field stores // whatever _methodPtr would have stored: and _methodPtr points to a // small thunk which removes the "this" pointer before going on // to _methodPtrAux. internal IntPtr _methodPtrAux; + // _methodPtr is a pointer to the method we will invoke + // It could be a small thunk if this is a static or UM call + internal IntPtr _methodPtr; + // This constructor is called from the class generated by the // compiler generated code [RequiresUnreferencedCode("The target method might be removed")] diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 4cd6f5abae7b2..f78401f14c39f 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -456,7 +456,7 @@ unsafe partial class CorInfoImpl { private const CORINFO_RUNTIME_ABI TargetABI = CORINFO_RUNTIME_ABI.CORINFO_CORECLR_ABI; - private uint OffsetOfDelegateFirstTarget => (uint)(2 * PointerSize); // Delegate::m_functionPointer + private uint OffsetOfDelegateFirstTarget => (uint)(3 * PointerSize); // Delegate::m_functionPointer private readonly ReadyToRunCodegenCompilation _compilation; private MethodWithGCInfo _methodCodeNode; diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index efd869681442f..900f4690199f3 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -3631,10 +3631,10 @@ void MethodContext::recGetThreadLocalStaticBlocksInfo(CORINFO_THREAD_STATIC_BLOC void MethodContext::dmpGetThreadLocalStaticBlocksInfo(DWORD key, const Agnostic_GetThreadLocalStaticBlocksInfo& value) { printf("GetThreadLocalStaticBlocksInfo key %u, tlsIndex-%s, " - ", tlsGetAddrFtnPtr-%016" PRIX64 ", tlsIndexObject - %016" PRIX64 + ", tlsGetAddrFtnPtr-%016" PRIX64 ", tlsIndexObject - %016" PRIX64 ", threadVarsSection - %016" PRIX64 ", offsetOfThreadLocalStoragePointer-%u, offsetOfMaxThreadStaticBlocks-%u" - ", offsetOfThreadStaticBlocks-%u, offsetOfGCDataPointer-%u", + ", offsetOfThreadStaticBlocks-%u, offsetOfGCDataPointer-%u", key, SpmiDumpHelper::DumpAgnostic_CORINFO_CONST_LOOKUP(value.tlsIndex).c_str(), value.tlsGetAddrFtnPtr, value.tlsIndexObject, value.threadVarsSection, value.offsetOfThreadLocalStoragePointer, value.offsetOfMaxThreadStaticBlocks, value.offsetOfThreadStaticBlocks, value.offsetOfGCDataPointer); @@ -3652,7 +3652,7 @@ void MethodContext::repGetThreadLocalStaticBlocksInfo(CORINFO_THREAD_STATIC_BLOC pInfo->tlsIndexObject = (void*)value.tlsIndexObject; pInfo->threadVarsSection = (void*)value.threadVarsSection; pInfo->offsetOfThreadLocalStoragePointer = value.offsetOfThreadLocalStoragePointer; - pInfo->offsetOfMaxThreadStaticBlocks = value.offsetOfMaxThreadStaticBlocks; + pInfo->offsetOfMaxThreadStaticBlocks = value.offsetOfMaxThreadStaticBlocks; pInfo->offsetOfThreadStaticBlocks = value.offsetOfThreadStaticBlocks; pInfo->offsetOfGCDataPointer = value.offsetOfGCDataPointer; } @@ -4370,7 +4370,7 @@ void MethodContext::repGetEEInfo(CORINFO_EE_INFO* pEEInfoOut) pEEInfoOut->offsetOfThreadFrame = (unsigned)0x10; pEEInfoOut->offsetOfGCState = (unsigned)0xc; pEEInfoOut->offsetOfDelegateInstance = (unsigned)0x8; - pEEInfoOut->offsetOfDelegateFirstTarget = (unsigned)0x10; + pEEInfoOut->offsetOfDelegateFirstTarget = (unsigned)0x18; pEEInfoOut->offsetOfWrapperDelegateIndirectCell = (unsigned)0x40; pEEInfoOut->sizeOfReversePInvokeFrame = (unsigned)0x8; pEEInfoOut->osPageSize = (size_t)0x1000; diff --git a/src/coreclr/vm/amd64/asmconstants.h b/src/coreclr/vm/amd64/asmconstants.h index 277fe6b23816b..f19d2812636e6 100644 --- a/src/coreclr/vm/amd64/asmconstants.h +++ b/src/coreclr/vm/amd64/asmconstants.h @@ -138,7 +138,7 @@ ASMCONSTANTS_C_ASSERT(OFFSETOF__ThreadExceptionState__m_pCurrentTracker -#define OFFSETOF__DelegateObject___methodPtr 0x10 +#define OFFSETOF__DelegateObject___methodPtr 0x18 ASMCONSTANT_OFFSETOF_ASSERT(DelegateObject, _methodPtr); #define OFFSETOF__DelegateObject___target 0x08 diff --git a/src/coreclr/vm/arm/asmconstants.h b/src/coreclr/vm/arm/asmconstants.h index 76b583c8e2893..16931168e3ce0 100644 --- a/src/coreclr/vm/arm/asmconstants.h +++ b/src/coreclr/vm/arm/asmconstants.h @@ -64,7 +64,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureSp == offsetof(LazyMachState, capture #define LazyMachState_captureIp (LazyMachState_captureSp+4) ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, captureIp)) -#define DelegateObject___methodPtr 0x08 +#define DelegateObject___methodPtr 0x0c ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x04 diff --git a/src/coreclr/vm/arm64/asmconstants.h b/src/coreclr/vm/arm64/asmconstants.h index 5b67217d47d4a..cc19728d9e29e 100644 --- a/src/coreclr/vm/arm64/asmconstants.h +++ b/src/coreclr/vm/arm64/asmconstants.h @@ -109,7 +109,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, capture #define VASigCookie__pNDirectILStub 0x8 ASMCONSTANTS_C_ASSERT(VASigCookie__pNDirectILStub == offsetof(VASigCookie, pNDirectILStub)) -#define DelegateObject___methodPtr 0x10 +#define DelegateObject___methodPtr 0x18 ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x08 diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 12b5b6b1d1ac2..b40a3b79117a2 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -247,14 +247,14 @@ DEFINE_METHOD(DATE_TIME, LONG_CTOR, .ctor, DEFINE_CLASS(DECIMAL, System, Decimal) DEFINE_METHOD(DECIMAL, CURRENCY_CTOR, .ctor, IM_Currency_RetVoid) -DEFINE_CLASS_U(System, Delegate, NoClass) -DEFINE_FIELD_U(_target, DelegateObject, _target) -DEFINE_FIELD_U(_methodPtr, DelegateObject, _methodPtr) -DEFINE_FIELD_U(_methodPtrAux, DelegateObject, _methodPtrAux) +DEFINE_CLASS_U(System, Delegate, NoClass) +DEFINE_FIELD_U(_target, DelegateObject, _target) +DEFINE_FIELD_U(_methodPtrAux, DelegateObject, _methodPtrAux) +DEFINE_FIELD_U(_methodPtr, DelegateObject, _methodPtr) DEFINE_CLASS(DELEGATE, System, Delegate) -DEFINE_FIELD(DELEGATE, TARGET, _target) -DEFINE_FIELD(DELEGATE, METHOD_PTR, _methodPtr) -DEFINE_FIELD(DELEGATE, METHOD_PTR_AUX, _methodPtrAux) +DEFINE_FIELD(DELEGATE, TARGET, _target) +DEFINE_FIELD(DELEGATE, METHOD_PTR_AUX, _methodPtrAux) +DEFINE_FIELD(DELEGATE, METHOD_PTR, _methodPtr) DEFINE_METHOD(DELEGATE, CONSTRUCT_DELEGATE, DelegateConstruct, IM_Obj_IntPtr_RetVoid) DEFINE_METHOD(DELEGATE, GET_INVOKE_METHOD, GetInvokeMethod, IM_RetIntPtr) DEFINE_METHOD(DELEGATE, SET_CACHED_METHOD, SetCachedMethod, IM_Obj_RetVoid) diff --git a/src/coreclr/vm/i386/asmconstants.h b/src/coreclr/vm/i386/asmconstants.h index 1ab9406b8ec6c..d766ee8591ea3 100644 --- a/src/coreclr/vm/i386/asmconstants.h +++ b/src/coreclr/vm/i386/asmconstants.h @@ -301,8 +301,8 @@ ASMCONSTANTS_C_ASSERT(InlinedCallFrame__m_pCalleeSavedFP == offsetof(InlinedCall #ifdef FEATURE_STUBS_AS_IL // DelegateObject from src/vm/object.h #define DelegateObject___target 0x04 // offset 0 is m_pMethTab of base class Object -#define DelegateObject___methodPtr 0x08 -#define DelegateObject___methodPtrAux 0x0c +#define DelegateObject___methodPtrAux 0x08 +#define DelegateObject___methodPtr 0x0c #define DelegateObject___invocationList 0x10 #define DelegateObject___invocationCount 0x14 diff --git a/src/coreclr/vm/loongarch64/asmconstants.h b/src/coreclr/vm/loongarch64/asmconstants.h index 2d47ceea2f267..e12d0040a74d0 100644 --- a/src/coreclr/vm/loongarch64/asmconstants.h +++ b/src/coreclr/vm/loongarch64/asmconstants.h @@ -134,7 +134,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, capture #define VASigCookie__pNDirectILStub 0x8 ASMCONSTANTS_C_ASSERT(VASigCookie__pNDirectILStub == offsetof(VASigCookie, pNDirectILStub)) -#define DelegateObject___methodPtr 0x10 +#define DelegateObject___methodPtr 0x18 ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x08 diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 5617cf84e9a01..8da977ba7fcaf 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1984,14 +1984,14 @@ class DelegateObject : public Object void SetTarget(OBJECTREF target) { WRAPPER_NO_CONTRACT; SetObjectReference(&_target, target); } static int GetOffsetOfTarget() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _target); } - PCODE GetMethodPtr() { LIMITED_METHOD_CONTRACT; return _methodPtr; } - void SetMethodPtr(PCODE methodPtr) { LIMITED_METHOD_CONTRACT; _methodPtr = methodPtr; } - static int GetOffsetOfMethodPtr() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodPtr); } - PCODE GetMethodPtrAux() { LIMITED_METHOD_CONTRACT; return _methodPtrAux; } void SetMethodPtrAux(PCODE methodPtrAux) { LIMITED_METHOD_CONTRACT; _methodPtrAux = methodPtrAux; } static int GetOffsetOfMethodPtrAux() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodPtrAux); } + PCODE GetMethodPtr() { LIMITED_METHOD_CONTRACT; return _methodPtr; } + void SetMethodPtr(PCODE methodPtr) { LIMITED_METHOD_CONTRACT; _methodPtr = methodPtr; } + static int GetOffsetOfMethodPtr() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodPtr); } + OBJECTREF GetInvocationList() { LIMITED_METHOD_CONTRACT; return _invocationList; } void SetInvocationList(OBJECTREF invocationList) { WRAPPER_NO_CONTRACT; SetObjectReference(&_invocationList, invocationList); } static int GetOffsetOfInvocationList() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _invocationList); } @@ -2008,16 +2008,16 @@ class DelegateObject : public Object private: // System.Delegate OBJECTREF _target; - PCODE _methodPtr; PCODE _methodPtrAux; + PCODE _methodPtr; // System.MulticastDelegate OBJECTREF _invocationList; INT_PTR _invocationCount; }; #define OFFSETOF__DelegateObject__target OBJECT_SIZE /* m_pMethTab */ -#define OFFSETOF__DelegateObject__methodPtr (OFFSETOF__DelegateObject__target + TARGET_POINTER_SIZE /* _target */) -#define OFFSETOF__DelegateObject__methodPtrAux (OFFSETOF__DelegateObject__methodPtr + TARGET_POINTER_SIZE /* _methodPtr */) +#define OFFSETOF__DelegateObject__methodPtrAux (OFFSETOF__DelegateObject__target + TARGET_POINTER_SIZE /* _target */) +#define OFFSETOF__DelegateObject__methodPtr (OFFSETOF__DelegateObject__methodPtrAux + TARGET_POINTER_SIZE /* _methodPtrAux */) #ifdef USE_CHECKED_OBJECTREFS typedef REF DELEGATEREF; diff --git a/src/coreclr/vm/riscv64/asmconstants.h b/src/coreclr/vm/riscv64/asmconstants.h index 57f9a63fbbd17..71095e3cffc99 100644 --- a/src/coreclr/vm/riscv64/asmconstants.h +++ b/src/coreclr/vm/riscv64/asmconstants.h @@ -129,7 +129,7 @@ ASMCONSTANTS_C_ASSERT(LazyMachState_captureIp == offsetof(LazyMachState, capture #define VASigCookie__pNDirectILStub 0x8 ASMCONSTANTS_C_ASSERT(VASigCookie__pNDirectILStub == offsetof(VASigCookie, pNDirectILStub)) -#define DelegateObject___methodPtr 0x10 +#define DelegateObject___methodPtr 0x18 ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); #define DelegateObject___target 0x08 From 90c6085a11f4e7d076a23810c0f85d321f4d3c56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Mon, 4 Mar 2024 17:03:48 +0100 Subject: [PATCH 10/20] Reorder asserts --- src/coreclr/vm/i386/asmconstants.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/i386/asmconstants.h b/src/coreclr/vm/i386/asmconstants.h index d766ee8591ea3..346da3308eff1 100644 --- a/src/coreclr/vm/i386/asmconstants.h +++ b/src/coreclr/vm/i386/asmconstants.h @@ -307,8 +307,8 @@ ASMCONSTANTS_C_ASSERT(InlinedCallFrame__m_pCalleeSavedFP == offsetof(InlinedCall #define DelegateObject___invocationCount 0x14 ASMCONSTANTS_C_ASSERT(DelegateObject___target == offsetof(DelegateObject, _target)); -ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtrAux == offsetof(DelegateObject, _methodPtrAux)); +ASMCONSTANTS_C_ASSERT(DelegateObject___methodPtr == offsetof(DelegateObject, _methodPtr)); ASMCONSTANTS_C_ASSERT(DelegateObject___invocationList == offsetof(DelegateObject, _invocationList)); ASMCONSTANTS_C_ASSERT(DelegateObject___invocationCount == offsetof(DelegateObject, _invocationCount)); From 95090234cf135ec8e92cee0066a891873bd0ff1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 4 Mar 2024 19:12:34 +0100 Subject: [PATCH 11/20] Fix contract again --- src/coreclr/vm/object.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index a5c7cbc82bac2..c37f3619243a4 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1398,7 +1398,7 @@ void DelegateObject::SetMethodBase(OBJECTREF newMethodBase) { CONTRACTL { - NOTHROW; + THROWS; GC_TRIGGERS; MODE_COOPERATIVE; } From 557bf4da6ab528df83eb2d740cbd7e741d53635c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 4 Mar 2024 21:27:40 +0100 Subject: [PATCH 12/20] GCPROTECT --- src/coreclr/vm/object.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index c37f3619243a4..78e93dbda64af 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1403,12 +1403,14 @@ void DelegateObject::SetMethodBase(OBJECTREF newMethodBase) MODE_COOPERATIVE; } CONTRACTL_END; - + + GCPROTECT_BEGIN(newMethodBase); PREPARE_NONVIRTUAL_CALLSITE(METHOD__DELEGATE__SET_CACHED_METHOD); DECLARE_ARGHOLDER_ARRAY(args, 2); args[ARGNUM_0] = PTR_TO_ARGHOLDER(this); args[ARGNUM_1] = OBJECTREF_TO_ARGHOLDER(newMethodBase); CALL_MANAGED_METHOD_NORET(args); + GCPROTECT_END(); } void StackTraceArray::Append(StackTraceElement const * begin, StackTraceElement const * end) From 2c16705a2d2adf82d98062b8e33b817de64c35c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Wed, 16 Oct 2024 20:41:02 +0200 Subject: [PATCH 13/20] Rework equality checks --- .../src/System/Delegate.CoreCLR.cs | 70 ++++++++++--------- .../src/System/MulticastDelegate.CoreCLR.cs | 16 +---- src/coreclr/debug/daccess/dacdbiimpl.cpp | 2 +- src/coreclr/vm/comdelegate.cpp | 24 ++++--- src/coreclr/vm/comdelegate.h | 4 +- src/coreclr/vm/object.cpp | 19 ----- src/coreclr/vm/object.h | 20 +++--- src/coreclr/vm/qcallentrypoints.cpp | 4 +- 8 files changed, 71 insertions(+), 88 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index 0a76d7f27109b..e63d836bde93f 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -17,15 +17,28 @@ public abstract partial class Delegate : ICloneable, ISerializable // _target is the object we will invoke on internal object? _target; // Initialized by VM as needed; null if static delegate + // VM handle to wrapped method, null when not initialized + private IntPtr _methodDesc; + + // _methodPtr is a pointer to the method we will invoke + // It could be a small thunk if this is a static or UM call + internal IntPtr _methodPtr; + // In the case of a static method passed to a delegate, this field stores // whatever _methodPtr would have stored: and _methodPtr points to a // small thunk which removes the "this" pointer before going on // to _methodPtrAux. internal IntPtr _methodPtrAux; - // _methodPtr is a pointer to the method we will invoke - // It could be a small thunk if this is a static or UM call - internal IntPtr _methodPtr; + internal IntPtr MethodDesc + { + get + { + if (_methodDesc == 0) + _methodDesc = GetMethodDesc(); + return _methodDesc; + } + } // This constructor is called from the class generated by the // compiler generated code @@ -126,11 +139,7 @@ public override bool Equals([NotNullWhen(true)] object? obj) // fall through method handle check } - // method ptrs don't match, go down long path - if (Cache.s_methodCache.TryGetValue(this, out object? thisCache) && thisCache is MethodInfo thisMethod && - Cache.s_methodCache.TryGetValue(d, out object? otherCache) && otherCache is MethodInfo otherMethod) - return thisMethod.Equals(otherMethod); - return InternalEqualMethodHandles(this, d); + return MethodDesc == d.MethodDesc; } public override int GetHashCode() @@ -151,21 +160,16 @@ public override int GetHashCode() return GetType().GetHashCode(); } - protected virtual MethodInfo GetMethodImpl() + protected MethodInfo GetMethodImpl() { - if (Cache.s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) - { - return methodInfo; - } - - return GetMethodImplUncached(); + return Cache.s_methodCache.TryGetValue(this, out MethodInfo? cachedValue) ? cachedValue : GetMethodImplUncached(); } - internal MethodInfo GetMethodImplUncached() + protected virtual MethodInfo GetMethodImplUncached() { - IRuntimeMethodInfo method = FindMethodHandle(); + IRuntimeMethodInfo method = CreateMethodInfo(MethodDesc); RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method); - + // need a proper declaring type instance method on a generic type if (declaringType.IsGenericType) { @@ -218,10 +222,10 @@ internal MethodInfo GetMethodImplUncached() return methodInfo; } - // this is called by the VM in addition to calls from managed code - internal void SetCachedMethod(object? value) + protected void SetCachedMethod(MethodInfo methodInfo) { - Cache.s_methodCache.AddOrUpdate(this, value); + Debug.Assert(methodInfo is not null); + Cache.s_methodCache.AddOrUpdate(this, methodInfo); } public object? Target => GetTarget(); @@ -531,25 +535,25 @@ internal unsafe IntPtr GetInvokeMethod() return (IntPtr)ptr; } - internal IRuntimeMethodInfo FindMethodHandle() + internal static IRuntimeMethodInfo CreateMethodInfo(IntPtr methodDesc) { - Delegate d = this; IRuntimeMethodInfo? methodInfo = null; - FindMethodHandle(ObjectHandleOnStack.Create(ref d), ObjectHandleOnStack.Create(ref methodInfo)); + CreateMethodInfo(methodDesc, ObjectHandleOnStack.Create(ref methodInfo)); return methodInfo!; } - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_FindMethodHandle")] - private static partial void FindMethodHandle(ObjectHandleOnStack d, ObjectHandleOnStack retMethodInfo); + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_CreateMethodInfo")] + private static partial void CreateMethodInfo(IntPtr methodDesc, ObjectHandleOnStack retMethodInfo); - private static bool InternalEqualMethodHandles(Delegate left, Delegate right) + private IntPtr GetMethodDesc() { - return InternalEqualMethodHandles(ObjectHandleOnStack.Create(ref left), ObjectHandleOnStack.Create(ref right)); + Delegate d = this; + return GetMethodDesc(ObjectHandleOnStack.Create(ref d)); } - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_InternalEqualMethodHandles")] + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_GetMethodDesc")] [return: MarshalAs(UnmanagedType.Bool)] - private static partial bool InternalEqualMethodHandles(ObjectHandleOnStack left, ObjectHandleOnStack right); + private static partial IntPtr GetMethodDesc(ObjectHandleOnStack instance); internal static IntPtr AdjustTarget(object target, IntPtr methodPtr) { @@ -574,11 +578,9 @@ internal void InitializeVirtualCallStub(IntPtr methodPtr) } // Caches MethodInfos, added either after first request or assigned from a DynamicMethod - // For open delegates to collectible types, this may contain a LoaderAllocator object - // that prevents the type from being unloaded. - internal static class Cache + private static class Cache { - public static readonly ConditionalWeakTable s_methodCache = new(); + public static readonly ConditionalWeakTable s_methodCache = new(); } } diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs index c12fe823f3d8d..6bafe85259a8a 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs @@ -494,17 +494,7 @@ public sealed override int GetHashCode() return base.GetTarget(); } - protected override MethodInfo GetMethodImpl() - { - if (Cache.s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo) - { - return methodInfo; - } - - return GetMulticastMethodImplUncached(); - } - - internal MethodInfo GetMulticastMethodImplUncached() + protected override MethodInfo GetMethodImplUncached() { if (_invocationCount != 0 && _invocationList != null) { @@ -525,7 +515,7 @@ internal MethodInfo GetMulticastMethodImplUncached() { // we handle unmanaged function pointers here because the generic ones (used for WinRT) would otherwise // be treated as open delegates by the base implementation, resulting in failure to get the MethodInfo - IRuntimeMethodInfo method = FindMethodHandle(); + IRuntimeMethodInfo method = CreateMethodInfo(MethodDesc); RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method); // need a proper declaring type instance method on a generic type @@ -542,7 +532,7 @@ internal MethodInfo GetMulticastMethodImplUncached() } // Otherwise, must be an inner delegate of a wrapper delegate of an open virtual method. In that case, call base implementation - return GetMethodImplUncached(); + return base.GetMethodImplUncached(); } // this should help inlining diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index 770e81a9480e1..3b0f008632d9d 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -3390,7 +3390,7 @@ HRESULT DacDbiInterfaceImpl::GetDelegateType(VMPTR_Object delegateObject, Delega // several pieces of logic so this replicates the logic mostly due to time constraints. The Mainly from: // - System.Private.CoreLib!System.Delegate.GetMethodImpl and System.Private.CoreLib!System.MulticastDelegate.GetMethodImpl // - System.Private.CoreLib!System.Delegate.GetTarget and System.Private.CoreLib!System.MulticastDelegate.GetTarget - // - coreclr!COMDelegate::GetMethodDesc and coreclr!COMDelegate::FindMethodHandle + // - coreclr!COMDelegate::GetMethodDesc and coreclr!COMDelegate::CreateMethodInfo // - coreclr!Delegate_Construct and the delegate type table in // - DELEGATE KINDS TABLE in comdelegate.cpp diff --git a/src/coreclr/vm/comdelegate.cpp b/src/coreclr/vm/comdelegate.cpp index 97af956c1f26d..80b4dc74cbf25 100644 --- a/src/coreclr/vm/comdelegate.cpp +++ b/src/coreclr/vm/comdelegate.cpp @@ -1090,6 +1090,8 @@ void COMDelegate::BindToMethod(DELEGATEREF *pRefThis, pTargetMethod->EnsureActive(); + refRealDelegate->SetMethodDesc(pTargetMethod); + if (fIsOpenDelegate) { _ASSERTE(pRefFirstArg == NULL || *pRefFirstArg == NULL); @@ -1190,7 +1192,7 @@ void COMDelegate::BindToMethod(DELEGATEREF *pRefThis, LoaderAllocator *pLoaderAllocator = pTargetMethod->GetLoaderAllocator(); if (pLoaderAllocator->IsCollectible()) - refRealDelegate->SetMethodBase(pLoaderAllocator->GetExposedObject()); + refRealDelegate->SetInvocationList(pLoaderAllocator->GetExposedObject()); GCPROTECT_END(); } @@ -1403,6 +1405,8 @@ OBJECTREF COMDelegate::ConvertToDelegate(LPVOID pCallback, MethodTable* pMT) // Wire up the unmanaged call stub to the delegate. delObj->SetTarget(delObj); // We are the "this" object + delObj->SetMethodDesc(pMD); + // For X86, we save the entry point in the delegate's method pointer and the UM Callsite in the aux pointer. delObj->SetMethodPtr(pMarshalStub); delObj->SetMethodPtrAux((PCODE)pCallback); @@ -1624,8 +1628,10 @@ extern "C" void QCALLTYPE Delegate_Construct(QCall::ObjectHandleOnStack _this, Q if (COMDelegate::NeedsWrapperDelegate(pMeth)) refThis = COMDelegate::CreateWrapperDelegate(refThis, pMeth); + refThis->SetMethodDesc(pMethOrig); + if (pMeth->GetLoaderAllocator()->IsCollectible()) - refThis->SetMethodBase(pMeth->GetLoaderAllocator()->GetExposedObject()); + refThis->SetInvocationList(pMeth->GetLoaderAllocator()->GetExposedObject()); // Open delegates. if (invokeArgCount == methodArgCount) @@ -2037,6 +2043,8 @@ DELEGATEREF COMDelegate::CreateWrapperDelegate(DELEGATEREF delegate, MethodDesc* // save the secure invoke stub. GetWrapperInvoke() can trigger GC. PCODE tmp = GetWrapperInvoke(pMD); + + gc.refWrapperDel->SetMethodDesc(pMD); gc.refWrapperDel->SetMethodPtr(tmp); // save the delegate MethodDesc for the frame gc.refWrapperDel->SetInvocationCount((INT_PTR)pMD); @@ -2051,7 +2059,7 @@ DELEGATEREF COMDelegate::CreateWrapperDelegate(DELEGATEREF delegate, MethodDesc* } // This method will get the MethodInfo for a delegate -extern "C" void QCALLTYPE Delegate_FindMethodHandle(QCall::ObjectHandleOnStack d, QCall::ObjectHandleOnStack retMethodInfo) +extern "C" void QCALLTYPE Delegate_CreateMethodInfo(MethodDesc* methodDesc, QCall::ObjectHandleOnStack retMethodInfo) { QCALL_CONTRACT; @@ -2059,26 +2067,24 @@ extern "C" void QCALLTYPE Delegate_FindMethodHandle(QCall::ObjectHandleOnStack d GCX_COOP(); - MethodDesc* pMD = COMDelegate::GetMethodDesc(d.Get()); + MethodDesc* pMD = methodDesc; pMD = MethodDesc::FindOrCreateAssociatedMethodDescForReflection(pMD, TypeHandle(pMD->GetMethodTable()), pMD->GetMethodInstantiation()); retMethodInfo.Set(pMD->GetStubMethodInfo()); END_QCALL; } -extern "C" BOOL QCALLTYPE Delegate_InternalEqualMethodHandles(QCall::ObjectHandleOnStack left, QCall::ObjectHandleOnStack right) +extern "C" MethodDesc* QCALLTYPE Delegate_GetMethodDesc(QCall::ObjectHandleOnStack instance) { QCALL_CONTRACT; - BOOL fRet = FALSE; + MethodDesc* fRet = nullptr; BEGIN_QCALL; GCX_COOP(); - MethodDesc* pMDLeft = COMDelegate::GetMethodDesc(left.Get()); - MethodDesc* pMDRight = COMDelegate::GetMethodDesc(right.Get()); - fRet = pMDLeft == pMDRight; + fRet = COMDelegate::GetMethodDesc(instance.Get()); END_QCALL; diff --git a/src/coreclr/vm/comdelegate.h b/src/coreclr/vm/comdelegate.h index be68b5ef40dd4..7df931c4b9d9f 100644 --- a/src/coreclr/vm/comdelegate.h +++ b/src/coreclr/vm/comdelegate.h @@ -127,9 +127,9 @@ extern "C" void QCALLTYPE Delegate_InternalAlloc(QCall::TypeHandle pType, QCall: extern "C" void QCALLTYPE Delegate_InternalAllocLike(QCall::ObjectHandleOnStack d); -extern "C" void QCALLTYPE Delegate_FindMethodHandle(QCall::ObjectHandleOnStack d, QCall::ObjectHandleOnStack retMethodInfo); +extern "C" void QCALLTYPE Delegate_CreateMethodInfo(MethodDesc* methodDesc, QCall::ObjectHandleOnStack retMethodInfo); -extern "C" BOOL QCALLTYPE Delegate_InternalEqualMethodHandles(QCall::ObjectHandleOnStack left, QCall::ObjectHandleOnStack right); +extern "C" MethodDesc* QCALLTYPE Delegate_GetMethodDesc(QCall::ObjectHandleOnStack instance); void DistributeEvent(OBJECTREF *pDelegate, diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 2efd6fc1bf121..050f16e09ad04 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1426,25 +1426,6 @@ void __fastcall ZeroMemoryInGCHeap(void* mem, size_t size) *memBytes++ = 0; } -void DelegateObject::SetMethodBase(OBJECTREF newMethodBase) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - GCPROTECT_BEGIN(newMethodBase); - PREPARE_NONVIRTUAL_CALLSITE(METHOD__DELEGATE__SET_CACHED_METHOD); - DECLARE_ARGHOLDER_ARRAY(args, 2); - args[ARGNUM_0] = PTR_TO_ARGHOLDER(this); - args[ARGNUM_1] = OBJECTREF_TO_ARGHOLDER(newMethodBase); - CALL_MANAGED_METHOD_NORET(args); - GCPROTECT_END(); -} - void StackTraceArray::Append(StackTraceElement const * elem) { CONTRACTL diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 6054f8ea23c4f..697227da77cc7 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1875,14 +1875,18 @@ class DelegateObject : public Object void SetTarget(OBJECTREF target) { WRAPPER_NO_CONTRACT; SetObjectReference(&_target, target); } static int GetOffsetOfTarget() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _target); } - PCODE GetMethodPtrAux() { LIMITED_METHOD_CONTRACT; return _methodPtrAux; } - void SetMethodPtrAux(PCODE methodPtrAux) { LIMITED_METHOD_CONTRACT; _methodPtrAux = methodPtrAux; } - static int GetOffsetOfMethodPtrAux() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodPtrAux); } + MethodDesc* GetMethodDesc() { LIMITED_METHOD_CONTRACT; return _methodDesc; } + void SetMethodDesc(MethodDesc* methodPtrAux) { LIMITED_METHOD_CONTRACT; _methodDesc = methodPtrAux; } + static int GetOffsetOfMethoddDesc() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodDesc); } PCODE GetMethodPtr() { LIMITED_METHOD_CONTRACT; return _methodPtr; } void SetMethodPtr(PCODE methodPtr) { LIMITED_METHOD_CONTRACT; _methodPtr = methodPtr; } static int GetOffsetOfMethodPtr() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodPtr); } + PCODE GetMethodPtrAux() { LIMITED_METHOD_CONTRACT; return _methodPtrAux; } + void SetMethodPtrAux(PCODE methodPtrAux) { LIMITED_METHOD_CONTRACT; _methodPtrAux = methodPtrAux; } + static int GetOffsetOfMethodPtrAux() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodPtrAux); } + OBJECTREF GetInvocationList() { LIMITED_METHOD_CONTRACT; return _invocationList; } void SetInvocationList(OBJECTREF invocationList) { WRAPPER_NO_CONTRACT; SetObjectReference(&_invocationList, invocationList); } static int GetOffsetOfInvocationList() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _invocationList); } @@ -1891,24 +1895,24 @@ class DelegateObject : public Object void SetInvocationCount(INT_PTR invocationCount) { LIMITED_METHOD_CONTRACT; _invocationCount = invocationCount; } static int GetOffsetOfInvocationCount() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _invocationCount); } - void SetMethodBase(OBJECTREF newMethodBase); - // README: // If you modify the order of these fields, make sure to update the definition in // BCL for this object. private: // System.Delegate OBJECTREF _target; - PCODE _methodPtrAux; + MethodDesc* _methodDesc; PCODE _methodPtr; + PCODE _methodPtrAux; // System.MulticastDelegate OBJECTREF _invocationList; INT_PTR _invocationCount; }; #define OFFSETOF__DelegateObject__target OBJECT_SIZE /* m_pMethTab */ -#define OFFSETOF__DelegateObject__methodPtrAux (OFFSETOF__DelegateObject__target + TARGET_POINTER_SIZE /* _target */) -#define OFFSETOF__DelegateObject__methodPtr (OFFSETOF__DelegateObject__methodPtrAux + TARGET_POINTER_SIZE /* _methodPtrAux */) +#define OFFSETOF__DelegateObject__methodDesc (OFFSETOF__DelegateObject__target + TARGET_POINTER_SIZE /* _target */) +#define OFFSETOF__DelegateObject__methodPtr (OFFSETOF__DelegateObject__methodDesc + TARGET_POINTER_SIZE /* _methodDesc */) +#define OFFSETOF__DelegateObject__methodPtrAux (OFFSETOF__DelegateObject__methodPtr + TARGET_POINTER_SIZE /* _methodPtr */) #ifdef USE_CHECKED_OBJECTREFS typedef REF DELEGATEREF; diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index dcc7df91acf2d..6722d9f382862 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -96,8 +96,8 @@ static const Entry s_QCall[] = DllImportEntry(Delegate_Construct) DllImportEntry(Delegate_InternalAlloc) DllImportEntry(Delegate_InternalAllocLike) - DllImportEntry(Delegate_FindMethodHandle) - DllImportEntry(Delegate_InternalEqualMethodHandles) + DllImportEntry(Delegate_CreateMethodInfo) + DllImportEntry(Delegate_GetMethodDesc) DllImportEntry(Environment_Exit) DllImportEntry(Environment_FailFast) DllImportEntry(Environment_GetProcessorCount) From bb2a509eca3d91f161738ccdb556011d364912e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Wed, 16 Oct 2024 21:48:26 +0200 Subject: [PATCH 14/20] Fix compile errors --- .../System.Private.CoreLib/src/System/Delegate.CoreCLR.cs | 7 +++---- .../src/System/MulticastDelegate.CoreCLR.cs | 8 ++++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index e63d836bde93f..1ede2f9579f33 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -160,12 +160,12 @@ public override int GetHashCode() return GetType().GetHashCode(); } - protected MethodInfo GetMethodImpl() + protected virtual MethodInfo GetMethodImpl() { return Cache.s_methodCache.TryGetValue(this, out MethodInfo? cachedValue) ? cachedValue : GetMethodImplUncached(); } - protected virtual MethodInfo GetMethodImplUncached() + internal virtual MethodInfo GetMethodImplUncached() { IRuntimeMethodInfo method = CreateMethodInfo(MethodDesc); RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method); @@ -222,7 +222,7 @@ protected virtual MethodInfo GetMethodImplUncached() return methodInfo; } - protected void SetCachedMethod(MethodInfo methodInfo) + internal void SetCachedMethod(MethodInfo methodInfo) { Debug.Assert(methodInfo is not null); Cache.s_methodCache.AddOrUpdate(this, methodInfo); @@ -552,7 +552,6 @@ private IntPtr GetMethodDesc() } [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_GetMethodDesc")] - [return: MarshalAs(UnmanagedType.Bool)] private static partial IntPtr GetMethodDesc(ObjectHandleOnStack instance); internal static IntPtr AdjustTarget(object target, IntPtr methodPtr) diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs index 6bafe85259a8a..b53c49c8185ea 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs @@ -494,7 +494,7 @@ public sealed override int GetHashCode() return base.GetTarget(); } - protected override MethodInfo GetMethodImplUncached() + internal override MethodInfo GetMethodImplUncached() { if (_invocationCount != 0 && _invocationList != null) { @@ -594,7 +594,7 @@ private void CtorCollectibleClosedStatic(object target, IntPtr methodPtr, IntPtr { this._target = target; this._methodPtr = methodPtr; - this.SetCachedMethod(GCHandle.InternalGet(gchandle)); + this._invocationList = GCHandle.InternalGet(gchandle); } [DebuggerNonUserCode] @@ -604,7 +604,7 @@ private void CtorCollectibleOpened(object target, IntPtr methodPtr, IntPtr shuff this._target = this; this._methodPtr = shuffleThunk; this._methodPtrAux = methodPtr; - this.SetCachedMethod(GCHandle.InternalGet(gchandle)); + this._invocationList = GCHandle.InternalGet(gchandle); } [DebuggerNonUserCode] @@ -613,7 +613,7 @@ private void CtorCollectibleVirtualDispatch(object target, IntPtr methodPtr, Int { this._target = this; this._methodPtr = shuffleThunk; - this.SetCachedMethod(GCHandle.InternalGet(gchandle)); + this._invocationList = GCHandle.InternalGet(gchandle); this.InitializeVirtualCallStub(methodPtr); } #pragma warning restore IDE0060 From 3bb047b60ca1ac7d8ab9aac083ca96d98db1f8dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Wed, 16 Oct 2024 21:53:04 +0200 Subject: [PATCH 15/20] Add asserts --- .../src/System/MulticastDelegate.CoreCLR.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs index b53c49c8185ea..ffe5cffa0f4ca 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs @@ -595,6 +595,7 @@ private void CtorCollectibleClosedStatic(object target, IntPtr methodPtr, IntPtr this._target = target; this._methodPtr = methodPtr; this._invocationList = GCHandle.InternalGet(gchandle); + Debug.Assert(InvocationListLogicallyNull()); } [DebuggerNonUserCode] @@ -605,6 +606,7 @@ private void CtorCollectibleOpened(object target, IntPtr methodPtr, IntPtr shuff this._methodPtr = shuffleThunk; this._methodPtrAux = methodPtr; this._invocationList = GCHandle.InternalGet(gchandle); + Debug.Assert(InvocationListLogicallyNull()); } [DebuggerNonUserCode] @@ -614,6 +616,7 @@ private void CtorCollectibleVirtualDispatch(object target, IntPtr methodPtr, Int this._target = this; this._methodPtr = shuffleThunk; this._invocationList = GCHandle.InternalGet(gchandle); + Debug.Assert(InvocationListLogicallyNull()); this.InitializeVirtualCallStub(methodPtr); } #pragma warning restore IDE0060 From b9573922b516fa548d5ae116e0286fbf6053d786 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Wed, 16 Oct 2024 22:12:19 +0200 Subject: [PATCH 16/20] Revert some changes, add more caching --- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 2 +- src/coreclr/vm/amd64/asmconstants.h | 8 ++++---- src/coreclr/vm/comdelegate.cpp | 12 ++++++++++-- src/coreclr/vm/corelib.h | 7 ++++--- src/coreclr/vm/object.h | 2 +- .../System.Private.CoreLib/src/System/Delegate.cs | 6 +++--- 6 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 5ebb9e14d2059..eebbbaceb7c2f 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -2111,7 +2111,7 @@ private void ceeInfoGetCallInfo( // of shared generic code calling a shared generic implementation method, which should be rare. // // An alternative design would be to add a new generic dictionary entry kind to hold the MethodDesc - // of the constrained target instead, and use that in some circumstances; however, implementation of + // of the constrained target instead, and use that in some circumstances; however, implementation of // that design requires refactoring variuos parts of the JIT interface as well as // TryResolveConstraintMethodApprox. In particular we would need to be abled to embed a constrained lookup // via EmbedGenericHandle, as well as decide in TryResolveConstraintMethodApprox if the call can be made diff --git a/src/coreclr/vm/amd64/asmconstants.h b/src/coreclr/vm/amd64/asmconstants.h index 7d1fb0d474069..8815f9ca8b354 100644 --- a/src/coreclr/vm/amd64/asmconstants.h +++ b/src/coreclr/vm/amd64/asmconstants.h @@ -631,13 +631,13 @@ template class FindCompileTimeConstant { private: - FindCompileTimeConstant(); + FindCompileTimeConstant(); }; void BogusFunction() { - // Sample usage to generate the error - FindCompileTimeConstant bogus_variable; - FindCompileTimeConstant bogus_variable2; + // Sample usage to generate the error + FindCompileTimeConstant bogus_variable; + FindCompileTimeConstant bogus_variable2; } #endif // defined(__cplusplus) && defined(USE_COMPILE_TIME_CONSTANT_FINDER) diff --git a/src/coreclr/vm/comdelegate.cpp b/src/coreclr/vm/comdelegate.cpp index b0fb6508d11ac..983c396866f9f 100644 --- a/src/coreclr/vm/comdelegate.cpp +++ b/src/coreclr/vm/comdelegate.cpp @@ -1823,11 +1823,16 @@ MethodDesc *COMDelegate::GetMethodDesc(OBJECTREF orDelegate) // If you modify this logic, please update DacDbiInterfaceImpl::GetDelegateType, DacDbiInterfaceImpl::GetDelegateType, // DacDbiInterfaceImpl::GetDelegateFunctionData, and DacDbiInterfaceImpl::GetDelegateTargetObject. - MethodDesc *pMethodHandle = NULL; - DELEGATEREF thisDel = (DELEGATEREF) orDelegate; DELEGATEREF innerDel = NULL; + MethodDesc *pMethodHandle = thisDel->GetMethodDesc(); + + if (pMethodHandle != NULL) + { + return pMethodHandle; + } + INT_PTR count = thisDel->GetInvocationCount(); if (count != 0) { @@ -1895,6 +1900,9 @@ MethodDesc *COMDelegate::GetMethodDesc(OBJECTREF orDelegate) } _ASSERTE(pMethodHandle); + + thisDel->SetMethodDesc(pMethodHandle); + return pMethodHandle; } diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 4573109f58444..c08e606f7be35 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -245,15 +245,16 @@ DEFINE_METHOD(DECIMAL, CURRENCY_CTOR, .ctor, DEFINE_CLASS_U(System, Delegate, NoClass) DEFINE_FIELD_U(_target, DelegateObject, _target) -DEFINE_FIELD_U(_methodPtrAux, DelegateObject, _methodPtrAux) +DEFINE_FIELD_U(_methodDesc, DelegateObject, _methodDesc) DEFINE_FIELD_U(_methodPtr, DelegateObject, _methodPtr) +DEFINE_FIELD_U(_methodPtrAux, DelegateObject, _methodPtrAux) DEFINE_CLASS(DELEGATE, System, Delegate) DEFINE_FIELD(DELEGATE, TARGET, _target) -DEFINE_FIELD(DELEGATE, METHOD_PTR_AUX, _methodPtrAux) +DEFINE_FIELD(DELEGATE, METHOD_DESC, _methodDesc) DEFINE_FIELD(DELEGATE, METHOD_PTR, _methodPtr) +DEFINE_FIELD(DELEGATE, METHOD_PTR_AUX, _methodPtrAux) DEFINE_METHOD(DELEGATE, CONSTRUCT_DELEGATE, DelegateConstruct, IM_Obj_IntPtr_RetVoid) DEFINE_METHOD(DELEGATE, GET_INVOKE_METHOD, GetInvokeMethod, IM_RetIntPtr) -DEFINE_METHOD(DELEGATE, SET_CACHED_METHOD, SetCachedMethod, IM_Obj_RetVoid) DEFINE_CLASS(INT128, System, Int128) DEFINE_CLASS(UINT128, System, UInt128) diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 7cf78bb87d0b4..c3841142a7ce4 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1877,7 +1877,7 @@ class DelegateObject : public Object MethodDesc* GetMethodDesc() { LIMITED_METHOD_CONTRACT; return _methodDesc; } void SetMethodDesc(MethodDesc* methodPtrAux) { LIMITED_METHOD_CONTRACT; _methodDesc = methodPtrAux; } - static int GetOffsetOfMethoddDesc() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodDesc); } + static int GetOffsetOfMethodDesc() { LIMITED_METHOD_CONTRACT; return offsetof(DelegateObject, _methodDesc); } PCODE GetMethodPtr() { LIMITED_METHOD_CONTRACT; return _methodPtr; } void SetMethodPtr(PCODE methodPtr) { LIMITED_METHOD_CONTRACT; _methodPtr = methodPtr; } diff --git a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs index 951fab4c1e912..a7133d3473a6c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs @@ -90,8 +90,8 @@ public abstract partial class Delegate : ICloneable, ISerializable /// The order of the delegates returned by the enumerator is the same order in which the current delegate invokes the methods that those delegates represent. /// The method returns an empty enumerator for null delegate. /// - public static Delegate.InvocationListEnumerator EnumerateInvocationList(TDelegate? d) where TDelegate : System.Delegate - => new Delegate.InvocationListEnumerator(Unsafe.As(d)); + public static System.Delegate.InvocationListEnumerator EnumerateInvocationList(TDelegate? d) where TDelegate : System.Delegate + => new InvocationListEnumerator(Unsafe.As(d)); /// /// Provides an enumerator for the invocation list of a delegate. @@ -135,7 +135,7 @@ public bool MoveNext() /// Implement IEnumerable.GetEnumerator() to return 'this' as the IEnumerator /// [EditorBrowsable(EditorBrowsableState.Never)] // Only here to make foreach work - public Delegate.InvocationListEnumerator GetEnumerator() => this; + public System.Delegate.InvocationListEnumerator GetEnumerator() => this; } public object? DynamicInvoke(params object?[]? args) From f3e48e15e5fbab81f068fbd225b7ef4ea6da61e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Thu, 17 Oct 2024 00:18:10 +0200 Subject: [PATCH 17/20] Add asserts --- src/coreclr/vm/comdelegate.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/comdelegate.cpp b/src/coreclr/vm/comdelegate.cpp index 983c396866f9f..7f145926e6e29 100644 --- a/src/coreclr/vm/comdelegate.cpp +++ b/src/coreclr/vm/comdelegate.cpp @@ -1293,6 +1293,7 @@ void COMDelegate::BindToMethod(DELEGATEREF *pRefThis, LoaderAllocator *pLoaderAllocator = pTargetMethod->GetLoaderAllocator(); + _ASSERTE(refRealDelegate->GetInvocationList() == NULL); if (pLoaderAllocator->IsCollectible()) refRealDelegate->SetInvocationList(pLoaderAllocator->GetExposedObject()); @@ -1731,6 +1732,7 @@ extern "C" void QCALLTYPE Delegate_Construct(QCall::ObjectHandleOnStack _this, Q refThis->SetMethodDesc(pMethOrig); + _ASSERTE(refThis->GetInvocationList() == NULL); if (pMeth->GetLoaderAllocator()->IsCollectible()) refThis->SetInvocationList(pMeth->GetLoaderAllocator()->GetExposedObject()); From 870107807f2e1dc58e886cc08a6c8a717afb79a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Thu, 17 Oct 2024 19:43:54 +0200 Subject: [PATCH 18/20] Improve GetHashCode --- .../src/System/Delegate.CoreCLR.cs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index 1ede2f9579f33..1449aec9474b7 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -144,20 +144,12 @@ public override bool Equals([NotNullWhen(true)] object? obj) public override int GetHashCode() { - // - // this is not right in the face of a method being jitted in one delegate and not in another - // in that case the delegate is the same and Equals will return true but GetHashCode returns a - // different hashcode which is not true. - /* - if (_methodPtrAux == IntPtr.Zero) - return unchecked((int)((long)this._methodPtr)); - else - return unchecked((int)((long)this._methodPtrAux)); - */ - if (_methodPtrAux == IntPtr.Zero) - return (_target != null ? RuntimeHelpers.GetHashCode(_target) * 33 : 0) + GetType().GetHashCode(); - else - return GetType().GetHashCode(); + int hashCode = _methodDesc.GetHashCode(); + if (_methodPtrAux == IntPtr.Zero && _target != null) + { + hashCode += RuntimeHelpers.GetHashCode(_target) * 33; + } + return hashCode; } protected virtual MethodInfo GetMethodImpl() From 63cba0a5eea86b0ce915edc5f1cd10c61f623bbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Thu, 17 Oct 2024 21:12:43 +0200 Subject: [PATCH 19/20] Fix GetHashCode --- .../System.Private.CoreLib/src/System/Delegate.CoreCLR.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index 1449aec9474b7..97771f8c41b13 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -144,7 +144,7 @@ public override bool Equals([NotNullWhen(true)] object? obj) public override int GetHashCode() { - int hashCode = _methodDesc.GetHashCode(); + int hashCode = MethodDesc.GetHashCode(); if (_methodPtrAux == IntPtr.Zero && _target != null) { hashCode += RuntimeHelpers.GetHashCode(_target) * 33; From 10964cd76c8629639b334cf3e5116b2b45ab5ecd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= Date: Fri, 18 Oct 2024 03:10:16 +0200 Subject: [PATCH 20/20] Massage codegen --- .../src/System/Delegate.CoreCLR.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs index 97771f8c41b13..8771c9c29373d 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs @@ -32,11 +32,10 @@ public abstract partial class Delegate : ICloneable, ISerializable internal IntPtr MethodDesc { + [MethodImpl(MethodImplOptions.AggressiveInlining)] get { - if (_methodDesc == 0) - _methodDesc = GetMethodDesc(); - return _methodDesc; + return _methodDesc != 0 ? _methodDesc : GetMethodDesc(); } } @@ -537,10 +536,13 @@ internal static IRuntimeMethodInfo CreateMethodInfo(IntPtr methodDesc) [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_CreateMethodInfo")] private static partial void CreateMethodInfo(IntPtr methodDesc, ObjectHandleOnStack retMethodInfo); + [MethodImpl(MethodImplOptions.NoInlining)] private IntPtr GetMethodDesc() { Delegate d = this; - return GetMethodDesc(ObjectHandleOnStack.Create(ref d)); + IntPtr desc = GetMethodDesc(ObjectHandleOnStack.Create(ref d)); + _methodDesc = desc; + return desc; } [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_GetMethodDesc")]