Make delegates immutable

src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs

@@ -17,9 +17,8 @@ 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
 
-        // 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
+        // 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
@@ -31,6 +30,15 @@ public abstract partial class Delegate : ICloneable, ISerializable
         // to _methodPtrAux.
         internal IntPtr _methodPtrAux;
 
+        internal IntPtr MethodDesc
+        {
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
+            get
+            {
+                return _methodDesc != 0 ? _methodDesc : GetMethodDesc();
+            }
+        }
+
         // This constructor is called from the class generated by the
         //  compiler generated code
         [RequiresUnreferencedCode("The target method might be removed")]
@@ -130,40 +138,27 @@ public override bool Equals([NotNullWhen(true)] object? obj)
                 // fall through method handle check
             }
 
-            // method ptrs don't match, go down long path
-
-            if (_methodBase is MethodInfo && d._methodBase is MethodInfo)
-                return _methodBase.Equals(d._methodBase);
-            else
-                return InternalEqualMethodHandles(this, d);
+            return MethodDesc == d.MethodDesc;
         }
 
         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()
         {
-            if (_methodBase is MethodInfo methodInfo)
-            {
-                return methodInfo;
-            }
+            return Cache.s_methodCache.TryGetValue(this, out MethodInfo? cachedValue) ? cachedValue : GetMethodImplUncached();
+        }
 
-            IRuntimeMethodInfo method = FindMethodHandle();
+        internal virtual MethodInfo GetMethodImplUncached()
+        {
+            IRuntimeMethodInfo method = CreateMethodInfo(MethodDesc);
             RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method);
 
             // need a proper declaring type instance method on a generic type
@@ -213,9 +208,15 @@ protected virtual MethodInfo GetMethodImpl()
                     }
                 }
             }
+            MethodInfo methodInfo = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
+            SetCachedMethod(methodInfo);
+            return methodInfo;
+        }
 
-            _methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
-            return (MethodInfo)_methodBase;
+        internal void SetCachedMethod(MethodInfo methodInfo)
+        {
+            Debug.Assert(methodInfo is not null);
+            Cache.s_methodCache.AddOrUpdate(this, methodInfo);
         }
 
         public object? Target => GetTarget();
@@ -525,25 +526,27 @@ 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)
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private IntPtr GetMethodDesc()
         {
-            return InternalEqualMethodHandles(ObjectHandleOnStack.Create(ref left), ObjectHandleOnStack.Create(ref right));
+            Delegate d = this;
+            IntPtr desc = GetMethodDesc(ObjectHandleOnStack.Create(ref d));
+            _methodDesc = desc;
+            return desc;
         }
 
-        [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_InternalEqualMethodHandles")]
-        [return: MarshalAs(UnmanagedType.Bool)]
-        private static partial bool InternalEqualMethodHandles(ObjectHandleOnStack left, ObjectHandleOnStack right);
+        [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Delegate_GetMethodDesc")]
+        private static partial IntPtr GetMethodDesc(ObjectHandleOnStack instance);
 
         internal static IntPtr AdjustTarget(object target, IntPtr methodPtr)
         {
@@ -566,6 +569,12 @@ internal void InitializeVirtualCallStub(IntPtr methodPtr)
         {
             return (_methodPtrAux == IntPtr.Zero) ? _target : null;
         }
+
+        // Caches MethodInfos, added either after first request or assigned from a DynamicMethod
+        private static class Cache
+        {
+            public static readonly ConditionalWeakTable<Delegate, MethodInfo> s_methodCache = new();
+        }
     }
 
     // These flags effect the way BindToMethodInfo and BindToMethodName are allowed to bind a delegate to a target method. Their

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
@@ -494,7 +494,7 @@ public sealed override int GetHashCode()
             return base.GetTarget();
         }
 
-        protected override MethodInfo GetMethodImpl()
+        internal override MethodInfo GetMethodImplUncached()
         {
             if (_invocationCount != 0 && _invocationList != null)
             {
@@ -515,12 +515,7 @@ 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 is MethodInfo methodInfo)
-                {
-                    return methodInfo;
-                }
-
-                IRuntimeMethodInfo method = FindMethodHandle();
+                IRuntimeMethodInfo method = CreateMethodInfo(MethodDesc);
                 RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method);
 
                 // need a proper declaring type instance method on a generic type
@@ -531,12 +526,13 @@ protected override MethodInfo GetMethodImpl()
                     declaringType = reflectedType;
                 }
 
-                _methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
-                return (MethodInfo)_methodBase;
+                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 base.GetMethodImplUncached();
         }
 
         // this should help inlining
@@ -598,7 +594,8 @@ private void CtorCollectibleClosedStatic(object target, IntPtr methodPtr, IntPtr
         {
             this._target = target;
             this._methodPtr = methodPtr;
-            this._methodBase = GCHandle.InternalGet(gchandle);
+            this._invocationList = GCHandle.InternalGet(gchandle);
+            Debug.Assert(InvocationListLogicallyNull());
         }
 
         [DebuggerNonUserCode]
@@ -608,7 +605,8 @@ private void CtorCollectibleOpened(object target, IntPtr methodPtr, IntPtr shuff
             this._target = this;
             this._methodPtr = shuffleThunk;
             this._methodPtrAux = methodPtr;
-            this._methodBase = GCHandle.InternalGet(gchandle);
+            this._invocationList = GCHandle.InternalGet(gchandle);
+            Debug.Assert(InvocationListLogicallyNull());
         }
 
         [DebuggerNonUserCode]
@@ -617,7 +615,8 @@ private void CtorCollectibleVirtualDispatch(object target, IntPtr methodPtr, Int
         {
             this._target = this;
             this._methodPtr = shuffleThunk;
-            this._methodBase = GCHandle.InternalGet(gchandle);
+            this._invocationList = GCHandle.InternalGet(gchandle);
+            Debug.Assert(InvocationListLogicallyNull());
             this.InitializeVirtualCallStub(methodPtr);
         }
 #pragma warning restore IDE0060

src/coreclr/debug/daccess/dacdbiimpl.cpp

@@ -3371,7 +3371,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
 

src/coreclr/vm/comdelegate.cpp

@@ -1204,6 +1204,8 @@ void COMDelegate::BindToMethod(DELEGATEREF   *pRefThis,
 
     pTargetMethod->EnsureActive();
 
+    refRealDelegate->SetMethodDesc(pTargetMethod);
+
     if (fIsOpenDelegate)
     {
         _ASSERTE(pRefFirstArg == NULL || *pRefFirstArg == NULL);
@@ -1291,8 +1293,9 @@ void COMDelegate::BindToMethod(DELEGATEREF   *pRefThis,
 
     LoaderAllocator *pLoaderAllocator = pTargetMethod->GetLoaderAllocator();
 
+    _ASSERTE(refRealDelegate->GetInvocationList() == NULL);
     if (pLoaderAllocator->IsCollectible())
-        refRealDelegate->SetMethodBase(pLoaderAllocator->GetExposedObject());
+        refRealDelegate->SetInvocationList(pLoaderAllocator->GetExposedObject());
 
     GCPROTECT_END();
 }
@@ -1505,6 +1508,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);
@@ -1725,8 +1730,11 @@ extern "C" void QCALLTYPE Delegate_Construct(QCall::ObjectHandleOnStack _this, Q
     if (COMDelegate::NeedsWrapperDelegate(pMeth))
         refThis = COMDelegate::CreateWrapperDelegate(refThis, pMeth);
 
+    refThis->SetMethodDesc(pMethOrig);
+
+    _ASSERTE(refThis->GetInvocationList() == NULL);
     if (pMeth->GetLoaderAllocator()->IsCollectible())
-        refThis->SetMethodBase(pMeth->GetLoaderAllocator()->GetExposedObject());
+        refThis->SetInvocationList(pMeth->GetLoaderAllocator()->GetExposedObject());
 
     // Open delegates.
     if (invokeArgCount == methodArgCount)
@@ -1817,11 +1825,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)
     {
@@ -1889,6 +1902,9 @@ MethodDesc *COMDelegate::GetMethodDesc(OBJECTREF orDelegate)
     }
 
     _ASSERTE(pMethodHandle);
+
+    thisDel->SetMethodDesc(pMethodHandle);
+
     return pMethodHandle;
 }
 
@@ -2132,6 +2148,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);
@@ -2146,34 +2164,32 @@ 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;
 
     BEGIN_QCALL;
 
     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;
 
@@ -2811,8 +2827,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.

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,