From e55812fa8a826fb04acc35e18e794abe1f787492 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 5 Feb 2024 07:39:03 +0100 Subject: [PATCH 1/6] Delete useless delegate virtual methods The split between `Delegate` and `MulticastDelegate` is a wart that likely has some history behind it. Types that inherit from `Delegate` directly would not be considered delegates by the runtime. They need to inherit `MulticastDelegate. I can't find a reason why we'd need some useless base implementation of these methods that immediately gets overriden in `MulticastDelegate`. This deletes the useless base implementation and moves the useful implementation from `MulticastDelegate` to `Delegate`. This along with #97951 saves ~40 bytes per delegate `MethodTable` because the virtual methods can now be devirtualized or placed into the sealed vtable. We might be able to do even more since technically sealed virtuals could be reshuffled after the codegen phase and slots eliminated then. --- .../src/System/Delegate.cs | 276 ++++++++++++++++- .../src/System/MulticastDelegate.cs | 281 ------------------ .../src/System/Delegate.cs | 4 + 3 files changed, 277 insertions(+), 284 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs index 6480d45d0f5f92..c7d042487c313d 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs @@ -1,18 +1,16 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.ComponentModel; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime; using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; using System.Runtime.Serialization; -using System.Text; using Internal.Reflection.Augments; using Internal.Runtime; -using Internal.Runtime.Augments; using Internal.Runtime.CompilerServices; namespace System @@ -411,5 +409,277 @@ internal static unsafe Delegate CreateDelegate(MethodTable* delegateEEType, IntP } return del; } + + private unsafe MulticastDelegate NewMulticastDelegate(Delegate[] invocationList, int invocationCount, bool thisIsMultiCastAlready = false) + { + // First, allocate a new multicast delegate just like this one, i.e. same type as the this object + MulticastDelegate result = (MulticastDelegate)RuntimeImports.RhNewObject(this.GetMethodTable()); + + // Performance optimization - if this already points to a true multicast delegate, + // copy _methodPtr and _methodPtrAux fields rather than calling into the EE to get them + if (thisIsMultiCastAlready) + { + result.m_functionPointer = this.m_functionPointer; + } + else + { + result.m_functionPointer = GetThunk(MulticastThunk); + } + result.m_firstParameter = result; + result.m_helperObject = invocationList; + result.m_extraFunctionPointerOrData = (IntPtr)invocationCount; + + return result; + } + + private static bool TrySetSlot(Delegate[] a, int index, Delegate o) + { + if (a[index] == null && System.Threading.Interlocked.CompareExchange(ref a[index], o, null) == null) + return true; + + // The slot may be already set because we have added and removed the same method before. + // Optimize this case, because it's cheaper than copying the array. + if (a[index] != null) + { + MulticastDelegate d = (MulticastDelegate)o; + MulticastDelegate dd = (MulticastDelegate)a[index]; + + if (object.ReferenceEquals(dd.m_firstParameter, d.m_firstParameter) && + object.ReferenceEquals(dd.m_helperObject, d.m_helperObject) && + dd.m_extraFunctionPointerOrData == d.m_extraFunctionPointerOrData && + dd.m_functionPointer == d.m_functionPointer) + { + return true; + } + } + return false; + } + + // This method will combine this delegate with the passed delegate + // to form a new delegate. + protected virtual Delegate CombineImpl(Delegate? d) + { + if (d is null) // cast to object for a more efficient test + return this; + + // Verify that the types are the same... + if (!InternalEqualTypes(this, d)) + throw new ArgumentException(SR.Arg_DlgtTypeMis); + + if (IsDynamicDelegate() && d.IsDynamicDelegate()) + { + throw new InvalidOperationException(); + } + + MulticastDelegate dFollow = (MulticastDelegate)d; + Delegate[]? resultList; + int followCount = 1; + Delegate[]? followList = dFollow.m_helperObject as Delegate[]; + if (followList != null) + followCount = (int)dFollow.m_extraFunctionPointerOrData; + + int resultCount; + Delegate[]? invocationList = m_helperObject as Delegate[]; + if (invocationList == null) + { + resultCount = 1 + followCount; + resultList = new Delegate[resultCount]; + resultList[0] = this; + if (followList == null) + { + resultList[1] = dFollow; + } + else + { + for (int i = 0; i < followCount; i++) + resultList[1 + i] = followList[i]; + } + return NewMulticastDelegate(resultList, resultCount); + } + else + { + int invocationCount = (int)m_extraFunctionPointerOrData; + resultCount = invocationCount + followCount; + resultList = null; + if (resultCount <= invocationList.Length) + { + resultList = invocationList; + if (followList == null) + { + if (!TrySetSlot(resultList, invocationCount, dFollow)) + resultList = null; + } + else + { + for (int i = 0; i < followCount; i++) + { + if (!TrySetSlot(resultList, invocationCount + i, followList[i])) + { + resultList = null; + break; + } + } + } + } + + if (resultList == null) + { + int allocCount = invocationList.Length; + while (allocCount < resultCount) + allocCount *= 2; + + resultList = new Delegate[allocCount]; + + for (int i = 0; i < invocationCount; i++) + resultList[i] = invocationList[i]; + + if (followList == null) + { + resultList[invocationCount] = dFollow; + } + else + { + for (int i = 0; i < followCount; i++) + resultList[invocationCount + i] = followList[i]; + } + } + return NewMulticastDelegate(resultList, resultCount, true); + } + } + + private Delegate[] DeleteFromInvocationList(Delegate[] invocationList, int invocationCount, int deleteIndex, int deleteCount) + { + Delegate[] thisInvocationList = (Delegate[])m_helperObject; + int allocCount = thisInvocationList.Length; + while (allocCount / 2 >= invocationCount - deleteCount) + allocCount /= 2; + + Delegate[] newInvocationList = new Delegate[allocCount]; + + for (int i = 0; i < deleteIndex; i++) + newInvocationList[i] = invocationList[i]; + + for (int i = deleteIndex + deleteCount; i < invocationCount; i++) + newInvocationList[i - deleteCount] = invocationList[i]; + + return newInvocationList; + } + + private static bool EqualInvocationLists(Delegate[] a, Delegate[] b, int start, int count) + { + for (int i = 0; i < count; i++) + { + if (!(a[start + i].Equals(b[i]))) + return false; + } + return true; + } + + // This method currently looks backward on the invocation list + // for an element that has Delegate based equality with value. (Doesn't + // look at the invocation list.) If this is found we remove it from + // this list and return a new delegate. If its not found a copy of the + // current list is returned. + protected virtual Delegate? RemoveImpl(Delegate d) + { + // There is a special case were we are removing using a delegate as + // the value we need to check for this case + // + MulticastDelegate? v = d as MulticastDelegate; + + if (v is null) + return this; + if (v.m_helperObject as Delegate[] == null) + { + Delegate[]? invocationList = m_helperObject as Delegate[]; + if (invocationList == null) + { + // they are both not real Multicast + if (this.Equals(v)) + return null; + } + else + { + int invocationCount = (int)m_extraFunctionPointerOrData; + for (int i = invocationCount; --i >= 0;) + { + if (v.Equals(invocationList[i])) + { + if (invocationCount == 2) + { + // Special case - only one value left, either at the beginning or the end + return invocationList[1 - i]; + } + else + { + Delegate[] list = DeleteFromInvocationList(invocationList, invocationCount, i, 1); + return NewMulticastDelegate(list, invocationCount - 1, true); + } + } + } + } + } + else + { + Delegate[]? invocationList = m_helperObject as Delegate[]; + if (invocationList != null) + { + int invocationCount = (int)m_extraFunctionPointerOrData; + int vInvocationCount = (int)v.m_extraFunctionPointerOrData; + for (int i = invocationCount - vInvocationCount; i >= 0; i--) + { + if (EqualInvocationLists(invocationList, v.m_helperObject as Delegate[], i, vInvocationCount)) + { + if (invocationCount - vInvocationCount == 0) + { + // Special case - no values left + return null; + } + else if (invocationCount - vInvocationCount == 1) + { + // Special case - only one value left, either at the beginning or the end + return invocationList[i != 0 ? 0 : invocationCount - 1]; + } + else + { + Delegate[] list = DeleteFromInvocationList(invocationList, invocationCount, i, vInvocationCount); + return NewMulticastDelegate(list, invocationCount - vInvocationCount, true); + } + } + } + } + } + + return this; + } + + public virtual Delegate[] GetInvocationList() + { + Delegate[] del; + Delegate[]? invocationList = m_helperObject as Delegate[]; + if (invocationList == null) + { + del = new Delegate[1]; + del[0] = this; + } + else + { + // Create an array of delegate copies and each + // element into the array + int invocationCount = (int)m_extraFunctionPointerOrData; + del = new Delegate[invocationCount]; + + for (int i = 0; i < del.Length; i++) + del[i] = invocationList[i]; + } + return del; + } + + [Obsolete(Obsoletions.LegacyFormatterImplMessage, DiagnosticId = Obsoletions.LegacyFormatterImplDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] + [EditorBrowsable(EditorBrowsableState.Never)] + public virtual void GetObjectData(SerializationInfo info, StreamingContext context) + { + throw new PlatformNotSupportedException(SR.Serialization_DelegatesNotSupported); + } } } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs index ce32047e542727..706520c5feb389 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs @@ -1,11 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.ComponentModel; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; -using System.Reflection; -using System.Runtime; using System.Runtime.CompilerServices; using System.Runtime.Serialization; @@ -138,272 +135,6 @@ public sealed override int GetHashCode() return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1); } - private unsafe MulticastDelegate NewMulticastDelegate(Delegate[] invocationList, int invocationCount, bool thisIsMultiCastAlready = false) - { - // First, allocate a new multicast delegate just like this one, i.e. same type as the this object - MulticastDelegate result = (MulticastDelegate)RuntimeImports.RhNewObject(this.GetMethodTable()); - - // Performance optimization - if this already points to a true multicast delegate, - // copy _methodPtr and _methodPtrAux fields rather than calling into the EE to get them - if (thisIsMultiCastAlready) - { - result.m_functionPointer = this.m_functionPointer; - } - else - { - result.m_functionPointer = GetThunk(MulticastThunk); - } - result.m_firstParameter = result; - result.m_helperObject = invocationList; - result.m_extraFunctionPointerOrData = (IntPtr)invocationCount; - - return result; - } - - private static bool TrySetSlot(Delegate[] a, int index, Delegate o) - { - if (a[index] == null && System.Threading.Interlocked.CompareExchange(ref a[index], o, null) == null) - return true; - - // The slot may be already set because we have added and removed the same method before. - // Optimize this case, because it's cheaper than copying the array. - if (a[index] != null) - { - MulticastDelegate d = (MulticastDelegate)o; - MulticastDelegate dd = (MulticastDelegate)a[index]; - - if (object.ReferenceEquals(dd.m_firstParameter, d.m_firstParameter) && - object.ReferenceEquals(dd.m_helperObject, d.m_helperObject) && - dd.m_extraFunctionPointerOrData == d.m_extraFunctionPointerOrData && - dd.m_functionPointer == d.m_functionPointer) - { - return true; - } - } - return false; - } - - - // This method will combine this delegate with the passed delegate - // to form a new delegate. - protected sealed override Delegate CombineImpl(Delegate? follow) - { - if (follow is null) // cast to object for a more efficient test - return this; - - // Verify that the types are the same... - if (!InternalEqualTypes(this, follow)) - throw new ArgumentException(SR.Arg_DlgtTypeMis); - - if (IsDynamicDelegate() && follow.IsDynamicDelegate()) - { - throw new InvalidOperationException(); - } - - MulticastDelegate dFollow = (MulticastDelegate)follow; - Delegate[]? resultList; - int followCount = 1; - Delegate[]? followList = dFollow.m_helperObject as Delegate[]; - if (followList != null) - followCount = (int)dFollow.m_extraFunctionPointerOrData; - - int resultCount; - Delegate[]? invocationList = m_helperObject as Delegate[]; - if (invocationList == null) - { - resultCount = 1 + followCount; - resultList = new Delegate[resultCount]; - resultList[0] = this; - if (followList == null) - { - resultList[1] = dFollow; - } - else - { - for (int i = 0; i < followCount; i++) - resultList[1 + i] = followList[i]; - } - return NewMulticastDelegate(resultList, resultCount); - } - else - { - int invocationCount = (int)m_extraFunctionPointerOrData; - resultCount = invocationCount + followCount; - resultList = null; - if (resultCount <= invocationList.Length) - { - resultList = invocationList; - if (followList == null) - { - if (!TrySetSlot(resultList, invocationCount, dFollow)) - resultList = null; - } - else - { - for (int i = 0; i < followCount; i++) - { - if (!TrySetSlot(resultList, invocationCount + i, followList[i])) - { - resultList = null; - break; - } - } - } - } - - if (resultList == null) - { - int allocCount = invocationList.Length; - while (allocCount < resultCount) - allocCount *= 2; - - resultList = new Delegate[allocCount]; - - for (int i = 0; i < invocationCount; i++) - resultList[i] = invocationList[i]; - - if (followList == null) - { - resultList[invocationCount] = dFollow; - } - else - { - for (int i = 0; i < followCount; i++) - resultList[invocationCount + i] = followList[i]; - } - } - return NewMulticastDelegate(resultList, resultCount, true); - } - } - - private Delegate[] DeleteFromInvocationList(Delegate[] invocationList, int invocationCount, int deleteIndex, int deleteCount) - { - Delegate[] thisInvocationList = (Delegate[])m_helperObject; - int allocCount = thisInvocationList.Length; - while (allocCount / 2 >= invocationCount - deleteCount) - allocCount /= 2; - - Delegate[] newInvocationList = new Delegate[allocCount]; - - for (int i = 0; i < deleteIndex; i++) - newInvocationList[i] = invocationList[i]; - - for (int i = deleteIndex + deleteCount; i < invocationCount; i++) - newInvocationList[i - deleteCount] = invocationList[i]; - - return newInvocationList; - } - - private static bool EqualInvocationLists(Delegate[] a, Delegate[] b, int start, int count) - { - for (int i = 0; i < count; i++) - { - if (!(a[start + i].Equals(b[i]))) - return false; - } - return true; - } - - // This method currently looks backward on the invocation list - // for an element that has Delegate based equality with value. (Doesn't - // look at the invocation list.) If this is found we remove it from - // this list and return a new delegate. If its not found a copy of the - // current list is returned. - protected sealed override Delegate? RemoveImpl(Delegate value) - { - // There is a special case were we are removing using a delegate as - // the value we need to check for this case - // - MulticastDelegate? v = value as MulticastDelegate; - - if (v is null) - return this; - if (v.m_helperObject as Delegate[] == null) - { - Delegate[]? invocationList = m_helperObject as Delegate[]; - if (invocationList == null) - { - // they are both not real Multicast - if (this.Equals(v)) - return null; - } - else - { - int invocationCount = (int)m_extraFunctionPointerOrData; - for (int i = invocationCount; --i >= 0;) - { - if (v.Equals(invocationList[i])) - { - if (invocationCount == 2) - { - // Special case - only one value left, either at the beginning or the end - return invocationList[1 - i]; - } - else - { - Delegate[] list = DeleteFromInvocationList(invocationList, invocationCount, i, 1); - return NewMulticastDelegate(list, invocationCount - 1, true); - } - } - } - } - } - else - { - Delegate[]? invocationList = m_helperObject as Delegate[]; - if (invocationList != null) - { - int invocationCount = (int)m_extraFunctionPointerOrData; - int vInvocationCount = (int)v.m_extraFunctionPointerOrData; - for (int i = invocationCount - vInvocationCount; i >= 0; i--) - { - if (EqualInvocationLists(invocationList, v.m_helperObject as Delegate[], i, vInvocationCount)) - { - if (invocationCount - vInvocationCount == 0) - { - // Special case - no values left - return null; - } - else if (invocationCount - vInvocationCount == 1) - { - // Special case - only one value left, either at the beginning or the end - return invocationList[i != 0 ? 0 : invocationCount - 1]; - } - else - { - Delegate[] list = DeleteFromInvocationList(invocationList, invocationCount, i, vInvocationCount); - return NewMulticastDelegate(list, invocationCount - vInvocationCount, true); - } - } - } - } - } - - return this; - } - - public sealed override Delegate[] GetInvocationList() - { - Delegate[] del; - Delegate[]? invocationList = m_helperObject as Delegate[]; - if (invocationList == null) - { - del = new Delegate[1]; - del[0] = this; - } - else - { - // Create an array of delegate copies and each - // element into the array - int invocationCount = (int)m_extraFunctionPointerOrData; - del = new Delegate[invocationCount]; - - for (int i = 0; i < del.Length; i++) - del[i] = invocationList[i]; - } - return del; - } - internal new bool HasSingleTarget => !(m_helperObject is Delegate[]); // Used by delegate invocation list enumerator @@ -418,17 +149,5 @@ public sealed override Delegate[] GetInvocationList() return ((uint)index < (uint)m_extraFunctionPointerOrData) ? invocationList[index] : null; } } - - protected override MethodInfo GetMethodImpl() - { - return base.GetMethodImpl(); - } - - [Obsolete(Obsoletions.LegacyFormatterImplMessage, DiagnosticId = Obsoletions.LegacyFormatterImplDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] - [EditorBrowsable(EditorBrowsableState.Never)] - public override void GetObjectData(SerializationInfo info, StreamingContext context) - { - throw new PlatformNotSupportedException(SR.Serialization_DelegatesNotSupported); - } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs index 1b861082530bf8..9621a6440b1c42 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs @@ -51,11 +51,13 @@ public abstract partial class Delegate : ICloneable, ISerializable public static Delegate CreateDelegate(Type type, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type target, string method) => CreateDelegate(type, target, method, ignoreCase: false, throwOnBindFailure: true)!; public static Delegate CreateDelegate(Type type, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type target, string method, bool ignoreCase) => CreateDelegate(type, target, method, ignoreCase, throwOnBindFailure: true)!; +#if !NATIVEAOT protected virtual Delegate CombineImpl(Delegate? d) => throw new MulticastNotSupportedException(SR.Multicast_Combine); protected virtual Delegate? RemoveImpl(Delegate d) => d.Equals(this) ? null : this; public virtual Delegate[] GetInvocationList() => new Delegate[] { this }; +#endif /// /// Gets a value that indicates whether the has a single invocation target. @@ -125,9 +127,11 @@ public bool MoveNext() return DynamicInvokeImpl(args); } +#if !NATIVEAOT [Obsolete(Obsoletions.LegacyFormatterImplMessage, DiagnosticId = Obsoletions.LegacyFormatterImplDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] [EditorBrowsable(EditorBrowsableState.Never)] public virtual void GetObjectData(SerializationInfo info, StreamingContext context) => throw new PlatformNotSupportedException(); +#endif public MethodInfo Method => GetMethodImpl(); From ce430d586f45f7409c26677f98e237207e705491 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 5 Feb 2024 09:59:31 -0800 Subject: [PATCH 2/6] Apply suggestions from code review --- .../System.Private.CoreLib/src/System/Delegate.cs | 7 ------- .../System.Private.CoreLib/src/System/Delegate.cs | 2 -- 2 files changed, 9 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs index c7d042487c313d..05df65d8156510 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs @@ -674,12 +674,5 @@ public virtual Delegate[] GetInvocationList() } return del; } - - [Obsolete(Obsoletions.LegacyFormatterImplMessage, DiagnosticId = Obsoletions.LegacyFormatterImplDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] - [EditorBrowsable(EditorBrowsableState.Never)] - public virtual void GetObjectData(SerializationInfo info, StreamingContext context) - { - throw new PlatformNotSupportedException(SR.Serialization_DelegatesNotSupported); - } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs index 9621a6440b1c42..9dcfea0365f186 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs @@ -127,11 +127,9 @@ public bool MoveNext() return DynamicInvokeImpl(args); } -#if !NATIVEAOT [Obsolete(Obsoletions.LegacyFormatterImplMessage, DiagnosticId = Obsoletions.LegacyFormatterImplDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] [EditorBrowsable(EditorBrowsableState.Never)] public virtual void GetObjectData(SerializationInfo info, StreamingContext context) => throw new PlatformNotSupportedException(); -#endif public MethodInfo Method => GetMethodImpl(); From ba07eed648e61745c910b0630f702ba266670ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 6 Feb 2024 00:52:02 +0100 Subject: [PATCH 3/6] FB --- .../src/System/Delegate.cs | 116 +++++++++++++++--- .../src/System/MulticastDelegate.cs | 100 +-------------- .../src/System/Delegate.cs | 2 +- 3 files changed, 102 insertions(+), 116 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs index 05df65d8156510..1c565a045b8265 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs @@ -40,10 +40,10 @@ protected Delegate([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Al // New Delegate Implementation - internal object m_firstParameter; - internal object m_helperObject; - internal nint m_extraFunctionPointerOrData; - internal IntPtr m_functionPointer; + private object m_firstParameter; + private object m_helperObject; + private nint m_extraFunctionPointerOrData; + private IntPtr m_functionPointer; // WARNING: These constants are also declared in System.Private.TypeLoader\Internal\Runtime\TypeLoader\CallConverterThunk.cs // Do not change their values without updating the values in the calling convention converter component @@ -241,11 +241,6 @@ private IntPtr GetActualTargetFunctionPointer(object thisObject) return OpenMethodResolver.ResolveMethod(m_extraFunctionPointerOrData, thisObject); } - public override int GetHashCode() - { - return GetType().GetHashCode(); - } - internal bool IsDynamicDelegate() { if (this.GetThunk(MulticastThunk) == IntPtr.Zero) @@ -282,14 +277,6 @@ protected virtual MethodInfo GetMethodImpl() return ReflectionAugments.ReflectionCoreCallbacks.GetDelegateMethod(this); } - public override bool Equals([NotNullWhen(true)] object? obj) - { - // It is expected that all real uses of the Equals method will hit the MulticastDelegate.Equals logic instead of this - // therefore, instead of duplicating the desktop behavior where direct calls to this Equals function do not behave - // correctly, we'll just throw here. - throw new PlatformNotSupportedException(); - } - public object Target { get @@ -674,5 +661,100 @@ public virtual Delegate[] GetInvocationList() } return del; } + + private bool InvocationListEquals(MulticastDelegate d) + { + Delegate[] invocationList = (Delegate[])m_helperObject; + if (d.m_extraFunctionPointerOrData != m_extraFunctionPointerOrData) + return false; + + int invocationCount = (int)m_extraFunctionPointerOrData; + for (int i = 0; i < invocationCount; i++) + { + Delegate dd = invocationList[i]; + Delegate[] dInvocationList = (Delegate[])d.m_helperObject; + if (!dd.Equals(dInvocationList[i])) + return false; + } + return true; + } + + public override bool Equals([NotNullWhen(true)] object? obj) + { + if (obj == null) + return false; + if (object.ReferenceEquals(this, obj)) + return true; + if (!InternalEqualTypes(this, obj)) + return false; + + // Since this is a MulticastDelegate and we know + // the types are the same, obj should also be a + // MulticastDelegate + Debug.Assert(obj is MulticastDelegate, "Shouldn't have failed here since we already checked the types are the same!"); + var d = Unsafe.As(obj); + + // there are 2 kind of delegate kinds for comparison + // 1- Multicast (m_helperObject is Delegate[]) + // 2- Single-cast delegate, which can be compared with a structural comparison + + IntPtr multicastThunk = GetThunk(MulticastThunk); + if (m_functionPointer == multicastThunk) + { + return d.m_functionPointer == multicastThunk && InvocationListEquals(d); + } + else + { + if (!object.ReferenceEquals(m_helperObject, d.m_helperObject) || + (!FunctionPointerOps.Compare(m_extraFunctionPointerOrData, d.m_extraFunctionPointerOrData)) || + (!FunctionPointerOps.Compare(m_functionPointer, d.m_functionPointer))) + { + return false; + } + + // Those delegate kinds with thunks put themselves into the m_firstParameter, so we can't + // blindly compare the m_firstParameter fields for equality. + if (object.ReferenceEquals(m_firstParameter, this)) + { + return object.ReferenceEquals(d.m_firstParameter, d); + } + + return object.ReferenceEquals(m_firstParameter, d.m_firstParameter); + } + } + + public override int GetHashCode() + { + Delegate[]? invocationList = m_helperObject as Delegate[]; + if (invocationList == null) + { + return base.GetHashCode(); + } + else + { + int hash = 0; + for (int i = 0; i < (int)m_extraFunctionPointerOrData; i++) + { + hash = hash * 33 + invocationList[i].GetHashCode(); + } + + return hash; + } + } + + public bool HasSingleTarget => !(m_helperObject is Delegate[]); + + // Used by delegate invocation list enumerator + internal Delegate? TryGetAt(int index) + { + if (!(m_helperObject is Delegate[] invocationList)) + { + return (index == 0) ? this : null; + } + else + { + return ((uint)index < (uint)m_extraFunctionPointerOrData) ? invocationList[index] : null; + } + } } } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs index 706520c5feb389..97a8150f3ffa2a 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs @@ -1,15 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using System.Runtime.Serialization; -using Internal.Runtime.CompilerServices; - namespace System { +#pragma warning disable CS0660, CS0661 // Defining operators, but not overriding Equals/GetHashCode public abstract class MulticastDelegate : Delegate, ISerializable { // This constructor is called from the class generated by the @@ -27,86 +25,6 @@ protected MulticastDelegate([DynamicallyAccessedMembers(DynamicallyAccessedMembe { } - private bool InvocationListEquals(MulticastDelegate d) - { - Delegate[] invocationList = (Delegate[])m_helperObject; - if (d.m_extraFunctionPointerOrData != m_extraFunctionPointerOrData) - return false; - - int invocationCount = (int)m_extraFunctionPointerOrData; - for (int i = 0; i < invocationCount; i++) - { - Delegate dd = invocationList[i]; - Delegate[] dInvocationList = (Delegate[])d.m_helperObject; - if (!dd.Equals(dInvocationList[i])) - return false; - } - return true; - } - - public sealed override bool Equals([NotNullWhen(true)] object? obj) - { - if (obj == null) - return false; - if (object.ReferenceEquals(this, obj)) - return true; - if (!InternalEqualTypes(this, obj)) - return false; - - // Since this is a MulticastDelegate and we know - // the types are the same, obj should also be a - // MulticastDelegate - Debug.Assert(obj is MulticastDelegate, "Shouldn't have failed here since we already checked the types are the same!"); - var d = Unsafe.As(obj); - - // there are 2 kind of delegate kinds for comparison - // 1- Multicast (m_helperObject is Delegate[]) - // 2- Single-cast delegate, which can be compared with a structural comparison - - IntPtr multicastThunk = GetThunk(MulticastThunk); - if (m_functionPointer == multicastThunk) - { - return d.m_functionPointer == multicastThunk && InvocationListEquals(d); - } - else - { - if (!object.ReferenceEquals(m_helperObject, d.m_helperObject) || - (!FunctionPointerOps.Compare(m_extraFunctionPointerOrData, d.m_extraFunctionPointerOrData)) || - (!FunctionPointerOps.Compare(m_functionPointer, d.m_functionPointer))) - { - return false; - } - - // Those delegate kinds with thunks put themselves into the m_firstParameter, so we can't - // blindly compare the m_firstParameter fields for equality. - if (object.ReferenceEquals(m_firstParameter, this)) - { - return object.ReferenceEquals(d.m_firstParameter, d); - } - - return object.ReferenceEquals(m_firstParameter, d.m_firstParameter); - } - } - - public sealed override int GetHashCode() - { - Delegate[]? invocationList = m_helperObject as Delegate[]; - if (invocationList == null) - { - return base.GetHashCode(); - } - else - { - int hash = 0; - for (int i = 0; i < (int)m_extraFunctionPointerOrData; i++) - { - hash = hash * 33 + invocationList[i].GetHashCode(); - } - - return hash; - } - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool operator ==(MulticastDelegate? d1, MulticastDelegate? d2) { @@ -134,20 +52,6 @@ public sealed override int GetHashCode() return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1); } - - internal new bool HasSingleTarget => !(m_helperObject is Delegate[]); - - // Used by delegate invocation list enumerator - internal Delegate? TryGetAt(int index) - { - if (!(m_helperObject is Delegate[] invocationList)) - { - return (index == 0) ? this : null; - } - else - { - return ((uint)index < (uint)m_extraFunctionPointerOrData) ? invocationList[index] : null; - } - } } +#pragma warning restore CS0660, CS0661 } diff --git a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs index 9dcfea0365f186..d24059770d5f88 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Delegate.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Delegate.cs @@ -57,13 +57,13 @@ public abstract partial class Delegate : ICloneable, ISerializable protected virtual Delegate? RemoveImpl(Delegate d) => d.Equals(this) ? null : this; public virtual Delegate[] GetInvocationList() => new Delegate[] { this }; -#endif /// /// Gets a value that indicates whether the has a single invocation target. /// /// true if the has a single invocation target. public bool HasSingleTarget => Unsafe.As(this).HasSingleTarget; +#endif /// /// Gets an enumerator for the invocation targets of this delegate. From 900add5b26970d35f9dc69ee0d428c188eeb558b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 6 Feb 2024 13:16:00 +0100 Subject: [PATCH 4/6] FB --- .../src/System/MulticastDelegate.cs | 45 +------------------ .../src/System.Private.CoreLib.csproj | 1 - .../System.Private.CoreLib.Shared.projitems | 1 + .../src/System/MulticastDelegate.cs | 0 .../src/System/MulticastDelegate.cs | 29 +----------- 5 files changed, 3 insertions(+), 73 deletions(-) rename src/{coreclr/nativeaot => libraries}/System.Private.CoreLib/src/System/MulticastDelegate.cs (100%) diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs index 5863897b75e48a..ab63d4cf83b74d 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs @@ -14,7 +14,7 @@ namespace System { [ClassInterface(ClassInterfaceType.None)] [ComVisible(true)] - public abstract class MulticastDelegate : Delegate + public abstract partial class MulticastDelegate : Delegate { // This is set under 2 circumstances // 1. Multicast delegate @@ -22,21 +22,6 @@ public abstract class MulticastDelegate : Delegate private object? _invocationList; // Initialized by VM as needed private nint _invocationCount; - // This constructor is called from the class generated by the - // compiler generated code (This must match the constructor - // in Delegate - [RequiresUnreferencedCode("The target method might be removed")] - protected MulticastDelegate(object target, string method) : base(target, method) - { - } - - // This constructor is called from a class to generate a - // delegate based upon a static method name and the Type object - // for the class defining the method. - protected MulticastDelegate([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type target, string method) : base(target, method) - { - } - internal bool IsUnmanagedFunctionPtr() { return _invocationCount == -1; @@ -448,34 +433,6 @@ public sealed override Delegate[] GetInvocationList() } } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static bool operator ==(MulticastDelegate? d1, MulticastDelegate? d2) - { - // Test d2 first to allow branch elimination when inlined for null checks (== null) - // so it can become a simple test - if (d2 is null) - { - return d1 is null; - } - - return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1); - } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static bool operator !=(MulticastDelegate? d1, MulticastDelegate? d2) - { - // Can't call the == operator as it will call object== - - // Test d2 first to allow branch elimination when inlined for not null checks (!= null) - // so it can become a simple test - if (d2 is null) - { - return d1 is not null; - } - - return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1); - } - public sealed override int GetHashCode() { if (IsUnmanagedFunctionPtr()) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj index e9e99968b9433b..4f7702c821c20d 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj @@ -183,7 +183,6 @@ - diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 9fc91793b70654..f15001de3fdf40 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -569,6 +569,7 @@ + diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.cs similarity index 100% rename from src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs rename to src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.cs diff --git a/src/mono/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/mono/System.Private.CoreLib/src/System/MulticastDelegate.cs index cd297e3b221982..e634afd23448d0 100644 --- a/src/mono/System.Private.CoreLib/src/System/MulticastDelegate.cs +++ b/src/mono/System.Private.CoreLib/src/System/MulticastDelegate.cs @@ -10,21 +10,10 @@ namespace System { [StructLayout(LayoutKind.Sequential)] - public abstract class MulticastDelegate : Delegate + public abstract partial class MulticastDelegate : Delegate { private Delegate[]? delegates; - [RequiresUnreferencedCode("The target method might be removed")] - protected MulticastDelegate(object target, string method) - : base(target, method) - { - } - - protected MulticastDelegate([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type target, string method) - : base(target, method) - { - } - [Obsolete(Obsoletions.LegacyFormatterImplMessage, DiagnosticId = Obsoletions.LegacyFormatterImplDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] [EditorBrowsable(EditorBrowsableState.Never)] public override void GetObjectData(SerializationInfo info, StreamingContext context) @@ -275,22 +264,6 @@ private static int LastIndexOf(Delegate[] haystack, Delegate[] needle) } } - public static bool operator ==(MulticastDelegate? d1, MulticastDelegate? d2) - { - if (d1 == null) - return d2 == null; - - return d1.Equals(d2); - } - - public static bool operator !=(MulticastDelegate? d1, MulticastDelegate? d2) - { - if (d1 == null) - return d2 != null; - - return !d1.Equals(d2); - } - internal override object? GetTarget() { return delegates?.Length > 0 ? delegates[delegates.Length - 1].GetTarget() : base.GetTarget(); From 3e281d036bfdd5d7a2f7d1145db8b7bc12e8d480 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 6 Feb 2024 14:07:35 +0100 Subject: [PATCH 5/6] Duh --- .../System.Private.CoreLib/src/System/MulticastDelegate.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.cs index 97a8150f3ffa2a..2508f2dc09f15a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.cs +++ b/src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.cs @@ -8,7 +8,7 @@ namespace System { #pragma warning disable CS0660, CS0661 // Defining operators, but not overriding Equals/GetHashCode - public abstract class MulticastDelegate : Delegate, ISerializable + public abstract partial class MulticastDelegate : Delegate, ISerializable { // This constructor is called from the class generated by the // compiler generated code (This must match the constructor From bf859d67abccef0681e3c0b647840d01ebab2217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 6 Feb 2024 23:12:59 +0100 Subject: [PATCH 6/6] FB --- .../System.Private.CoreLib/System.Private.CoreLib.csproj | 2 +- .../{MulticastDelegate.cs => MulticastDelegate.CoreCLR.cs} | 0 src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj | 2 +- .../System/{MulticastDelegate.cs => MulticastDelegate.Mono.cs} | 0 4 files changed, 2 insertions(+), 2 deletions(-) rename src/coreclr/System.Private.CoreLib/src/System/{MulticastDelegate.cs => MulticastDelegate.CoreCLR.cs} (100%) rename src/mono/System.Private.CoreLib/src/System/{MulticastDelegate.cs => MulticastDelegate.Mono.cs} (100%) diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj index 08f6699e0a02db..4afe69cdc34e17 100644 --- a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -151,7 +151,7 @@ - + diff --git a/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs similarity index 100% rename from src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs rename to src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs diff --git a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj index cf157fee87514c..49c5d602711719 100644 --- a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -172,7 +172,7 @@ - + diff --git a/src/mono/System.Private.CoreLib/src/System/MulticastDelegate.cs b/src/mono/System.Private.CoreLib/src/System/MulticastDelegate.Mono.cs similarity index 100% rename from src/mono/System.Private.CoreLib/src/System/MulticastDelegate.cs rename to src/mono/System.Private.CoreLib/src/System/MulticastDelegate.Mono.cs