Share common delegate code, cleanup NativeAOT delegate impl

[MichalPetryka]

Continues the previous cleanups, uses the current CoreCLR impl as baseline for shared code.

Also adopts newly added helpers for Mono.

cc @jkotas @MichalStrehovsky

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[MichalPetryka]

@MihuBot -nuget

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "eab25cd9ba41f32642710eb741a82782379dc989",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "840cea322a2978a568e8cdfd4d2e643fcf42aa1c",
  "last_reviewed_commit": "eab25cd9ba41f32642710eb741a82782379dc989",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "840cea322a2978a568e8cdfd4d2e643fcf42aa1c",
  "last_recorded_worker_run_id": "29761352259",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "eab25cd9ba41f32642710eb741a82782379dc989",
      "review_id": 4737166670
    }
  ]
}

[Copilot]

Pull request overview

This PR consolidates delegate implementation logic across runtimes by moving common invocation-list / equality / combine-remove code into the shared System.Delegate implementation, and introducing a shared helper for runtime type-equivalence checks. It updates Mono, CoreCLR, and NativeAOT delegate code to use these shared helpers and removes duplicated platform-specific implementations.

Changes:

• Add RuntimeHelpers.TypeEquivalent(object a, object b) to centralize delegate type identity / equivalence checks across runtimes.
• Move shared delegate functionality (invocation list enumeration, CombineImpl / RemoveImpl, Equals, GetInvocationList, etc.) into src/libraries/.../Delegate.cs under #if !MONO, and trim corresponding CoreCLR/NativeAOT-specific duplicates.
• Update Mono delegate implementations to use the shared GetInvokeMethod helper and RuntimeHelpers.TypeEquivalent.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/mono/System.Private.CoreLib/src/System/MulticastDelegate.Mono.cs	Switches combine-time type check to shared RuntimeHelpers.TypeEquivalent.
src/mono/System.Private.CoreLib/src/System/Delegate.Mono.cs	Uses shared GetInvokeMethod and RuntimeHelpers.TypeEquivalent; removes now-redundant local type-compare helper.
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs	Introduces shared RuntimeHelpers.TypeEquivalent helper (with MONO/CoreCLR/NATIVEAOT conditional behavior).
src/libraries/System.Private.CoreLib/src/System/Delegate.cs	Centralizes shared delegate logic (invocation list handling, combine/remove, equality) for non-Mono builds; adds GetInvokeMethod helper.
src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs	Removes duplicated shared logic and provides CoreCLR-specific SlotEquals / EqualsCore pieces consumed by shared code.
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs	Removes duplicated shared logic and aligns NativeAOT delegate implementation with shared invocation-list / equality plumbing.

[github-actions]

Holistic Review

Motivation: Delegate implementations are duplicated across CoreCLR, NativeAOT, and Mono. This PR continues an ongoing cleanup effort, using the current CoreCLR implementation as the baseline for shared code, moving the invocation-list, equality, combine/remove, and hashing logic into the shared System.Delegate partial and adopting the newly added helpers for Mono. The goal is less duplication and a single source of truth for the multicast/Wrapper[] machinery.

Approach: CombineImpl, RemoveImpl, GetInvocationList, TryGetAt, Equals, the Wrapper struct, TrySetSlot, and DeleteFromInvocationList are hoisted into src/libraries/.../Delegate.cs under #if !MONO. Per-runtime files keep only the fields, thunk constants, EqualsCore, SlotEquals, NewMulticastDelegate, and TryGetInvocations that genuinely differ. A new RuntimeHelpers.TypeEquivalent centralizes the former per-runtime InternalEqualTypes (MethodTable identity + optional FEATURE_TYPEEQUIVALENCE QCall on CoreCLR/NativeAOT, simple GetType() comparison on Mono). A shared Delegate.GetInvokeMethod replaces scattered GetType().GetMethod("Invoke") calls, carrying the trimmer suppression once. The NativeAOT file additionally receives substantial non-functional cleanup (field/const reordering, control-flow flattening, default simplifications) and the hashing was reworked to fold the MethodTable into HashCode.Combine.

Summary: This is a well-structured consolidation that meaningfully reduces duplication and, encouragingly, keeps the delicate lock-free TrySetSlot/CompareExchange combine path and the multicast special-cases intact and identical in shape across runtimes. The shared Wrapper.Equals correctly uses EqualsCore (types are known-equal in that context) while the public Equals retains the TypeEquivalent gate, preserving COM type-equivalence semantics. The GetHashCode change on NativeAOT now mixes the MethodTable into the hash, which is a semantic change but only affects hash distribution, not correctness. Two things warrant author confirmation: (1) the dropped IsDynamicDelegate/InvalidOperationException guard in NativeAOT CombineImpl (inline comment), and (2) broad reliance on Debug.Assert in the new Wrapper and TypeEquivalent helpers, which is consistent with the surrounding code. Because this is core delegate infrastructure with subtle cross-runtime and threading behavior, the ultimate safety net is the existing delegate/multicast test suite plus NativeAOT and Mono CI legs; the changes read as behavior-preserving apart from the one flagged NativeAOT guard. Overall this looks like a solid, largely mechanical refactor with a single point I'd want the author to double-check.

Detailed Findings

• NativeAOT CombineImpl dropped the dynamic-delegate guard (inline comment on src/libraries/System.Private.CoreLib/src/System/Delegate.cs): the previous NativeAOT implementation fast-failed with InvalidOperationException when combining a dynamic delegate; the shared implementation does not. Please confirm this path is unreachable, otherwise it is a NativeAOT behavior regression.

• Non-actionable observations: The extensive reformatting/reflowing in the NativeAOT file (const reordering, default simplifications, control-flow flattening, GetDelegateLdFtnResult inversion) appears semantically equivalent on read; the inverted condition in GetDelegateLdFtnResult (!= _methodPtr early-return) was verified to preserve the original open-resolver vs. plain-pointer outcomes. The TypeEquivalent helper correctly mirrors the prior per-runtime behavior including the Mono GetType()-only path and the FEATURE_TYPEEQUIVALENCE/!FEATURE_TYPEEQUIVALENCE split. No action needed.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 152.7 AIC · ⌖ 11.2 AIC · ⊞ 10K

[MichalPetryka]

@jkotas Do you maybe know what's with the mono failures? The stacktraces there are wrong (unless return d; can throw ArgumentException but that'd be a first) and I dont see anything that'd break from the mono changes I did.

[MichalPetryka]

@jkotas Do you maybe know what's with the mono failures? The stacktraces there are wrong (unless return d; can throw ArgumentException but that'd be a first) and I dont see anything that'd break from the mono changes I did.

Seems the mono failures went away when I made it find NonPublic Invokes too.

[MichalPetryka]

@MihuBot -nuget

[MichalPetryka]

@jkotas This seems good for review now, NAOT issue and Mono failures are solved.

[MichalPetryka]

Suggested change

	#if MONO
	// Mono doesn't have MethodTables nor Equivalence
	return a.GetType() == b.GetType();
	#else
	#if MONO
	// Mono doesn't have MethodTables nor Equivalence
	return a.GetType() == b.GetType();
	#else

Should this be always used when !FEATURE_TYPEEQUIVALENCE?

[MichalPetryka]

@MichalStrehovsky This seems to have ~4KB size regressions on NAOT, probably due to LastIndexOf. Is that fine or should I look into reducing it?

[jkotas]

This seems to have ~4KB size regressions on NAOT, probably due to LastIndexOf. Is that fine or should I look into reducing it?

Are you able to measure any performance gains from use of LastIndexOf and SequenceEquals? I doubt you will be able to. There methods are heavily optimized for Equals operations that compile down into like a single instruction, at the cost of generating code bloat. Delegate.Equals is not anything like that. So you are paying for the code bloat and gain nothing in return. Plus you are probably paying some dead weight for implementation of the generic interface on the Wrapper type.

I think it is better to keep the trivial local implementations of LastIndexOf and SequenceEquals (for both NAOT and regular CoreCLR).

[tannergooding]

I think it is better to keep the trivial local implementations of LastIndexOf and SequenceEquals (for both NAOT and regular CoreCLR).

Agreed.

---

Separately, however, it might be worth investigating why there's so much "bloat" brought in for reference types here. Those should have the vector and all the special fast paths elided as dead code.

This should largely just hit this:

public static unsafe bool SequenceEqual<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> other) where T : IEquatable<T>?
{
    int length = span.Length;
    int otherLength = other.Length;

    if (RuntimeHelpers.IsBitwiseEquatable<T>())
    {
        return length == otherLength &&
            SpanHelpers.SequenceEqual(
                ref Unsafe.As<T, byte>(ref MemoryMarshal.GetReference(span)),
                ref Unsafe.As<T, byte>(ref MemoryMarshal.GetReference(other)),
                ((uint)otherLength) * (nuint)sizeof(T));  // If this multiplication overflows, the Span we got overflows the entire address range. There's no happy outcome for this API in such a case so we choose not to take the overhead of checking.
    }

    return length == otherLength && SpanHelpers.SequenceEqual(ref MemoryMarshal.GetReference(span), ref MemoryMarshal.GetReference(other), length);
}

Which should treat the entire bitwise equatable path as dead code and then hit SpanHelpers.SequenceEqual and that should just do the checks, unrolled while loop, and trailing handling.

There's more code here than the trivial while loop, but 4KB of codegen increase seems a bit excessive. The loop body is also more complex than it probably needs to be for the reference type path (and more unsafe than it needs to be).

[MichalPetryka]

Are you able to measure any performance gains from use of LastIndexOf and SequenceEquals? I doubt you will be able to.

The motivation for me using them here was to simplify the code by not keeping custom local variant, I was not concerned with performance when doing so.

I'd prefer to keep using them for code clarity but I can add local helpers, just the sequence based LastIndexOf one will not be very pretty.