Optimize and simplify delegate layout

[MichalPetryka]

First attempt at making delegate GC fields immutable in CoreCLR so that they can be allocated on the NonGC heap.

I've checked it with a simple app and corerun locally with a delegate from an unloadable ALC and it seemed to not crash, assert nor unload the ALC from under the delegate, however I couldn't actually find any runtime tests that would verify delegates from unloadable ALCs work so the CI coverage might be missing.

One small point of concern is that this might make delegate equality checks slower since they rely on checking the methods in the last "slow path" check, which is however always hit for different delegates AFAIR.

Contributes to #85014.

cc @jkotas

[MichalPetryka]

I'm not sure what's up with the failures here, tests that are failing on the CI seem to pass on my machine.
EDIT: I was testing with R2R disabled locally since VS kept complaining about being unable to load PDBs for it.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jkotas]

Could you please collect some perf numbers to give us an idea about the improvements and regressions in the affected areas? We may want to do some optimizations to mitigate the regressions.

[jkotas]

One small point of concern is that this might make delegate equality checks slower since they rely on checking the methods in the last "slow path" check, which is however always hit for different delegates AFAIR.

The existing code tries to compare MethodInfos as a cheap fast path. Most delegates do not have cached MethodInfo, so this fast path is hit rarely - but it is very cheap, so it is still worth it.

This cheap fast path is not cheap anymore with this change. It may be best to delete the fast path that is trying to compare the MethodInfos and potentially optimize Delegate_InternalEqualMethodHandles instead.

[MichalPetryka]

Could you please collect some perf numbers to give us an idea about the improvements and regressions in the affected areas? We may want to do some optimizations to mitigate the regressions.

I think the thing that'd need benchmarking here are equality checks and maybe the impact of collectible delegates being stored in the CWT on the GC, the rest of things shouldn't be performance sensitive enough to matter I think? I'm not fully sure what'd be the proper way for benchmarking the latter.

This cheap fast path is not cheap anymore with this change. It may be best to delete the fast path that is trying to compare the MethodInfos and potentially optimize Delegate_InternalEqualMethodHandles instead.

I am going to benchmark the impact of the equality change tomorrow, I feel like if the impact won't be big, potential optimizations to it can be done later.

[jkotas]

I'm not fully sure what'd be the proper way for benchmarking the latter.

Write a small program that loads an assembly as collectible and calls a method in it. The method in collectible assembly can create delegates in a loop. (If you would like to do it under benchmarknet, it works too - but it is probably more work.)

[max-charlamb]

We should document the -1 as a contract constant at the top of the file.

See Contract Constants section in other datacontract doc files like Loader.md for reference:

### Contract Constants:
| Name | Type | Purpose | Value |
| --- | --- | --- | --- |
| `ASSEMBLY_NOTIFYFLAGS_PROFILER_NOTIFIED` | uint | Flag in Assembly NotifyFlags indicating the Assembly will notify profilers. | `0x1` |

[MichalPetryka]

Does it look good now?

[jkotas]

@max-charlamb

[MichalPetryka]

I am not happy about refactors in this PR that are introducing unsafe code that can be done with safe code instead. I would be fine with no refactors; or refactors using safe code.

Do you mean the unsafe code in TryGetInvocations here? AFAIR the Unsafe.As currently can't be avoided since we can't upcast for span, GetArrayDataReference and CreateReadOnlySpan can be replaced but they'll then introduce 2 bounds checks since the JIT doesn't know that array.Length >= invocation count > 0 so I'd prefer to keep them as this helper is used in performance sensitive paths like Equals.

If you prefer though, I can remove GetArrayDataReference and CreateReadOnlySpan here and move them to a 2nd PR to show diffs from them.

EDIT: CreateReadOnlySpan can't be avoided either cause we can't Unsafe.As the array itself.

[jkotas]

The code in main does not have the unsafe array casts or bounds check removals via unsafe code. The refactoring that introduces the extra unsafe code is a performance optimization that is not strictly related to the layout changes.

The unsafe array casts can be eliminated by using the struct wrapper like in NAOT, and I do not mind an extra bound check for Equals - I expect that it will barely register in Equals performance.

[MichalPetryka]

@jkotas Is the last commit what you intended?

[MichalPetryka]

@EgorBot -amd -arm

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;
using System;
using System.Reflection;
using System.Reflection.Emit;

public class Benchmarks
{
    public Action a;
    public Action b1;
    public Action b2;
    public Action d;

    public Action m1;
    public Action m2;

    public static ulong Counter;

    [GlobalSetup]
    public void Setup()
    {
        a = new(A);
        b1 = new(B);
        b2 = new(B);
        DynamicMethod method = new("D", typeof(void), [], typeof(Benchmarks).Module, true);
        ILGenerator il = method.GetILGenerator();
        il.Emit(OpCodes.Ret);

        d = method.CreateDelegate<Action>();

        m1 = a;
        m1 += b1;

        m2 = a;
        m1 += b2;
    }

    public void A() { }
    public void B() { }

    [Benchmark]
    public MethodInfo Method() => a.Method;

    [Benchmark]
    public MethodInfo MethodMulticast() => m1.Method;

    [Benchmark]
    public MethodInfo MethodDynamic() => d.Method;

    [Benchmark]
    public object Target() => a.Target;

    [Benchmark]
    public object TargetMulticast() => m1.Target;

    [Benchmark]
    public object TargetDynamic() => d.Target;

    [Benchmark]
    public bool EqualsTrue() => b1 == b2;

    [Benchmark]
    public bool EqualsFalse() => a == b1;

    [Benchmark]
    public bool EqualsMulticast() => m1 == m2;

    [Benchmark]
    public int GetHashCode() => a.GetHashCode();

    [Benchmark]
    public Action DynamicCreate()
    {
        DynamicMethod method = new($"D{Counter++}", typeof(void), [], typeof(Benchmarks).Module, true);
        ILGenerator il = method.GetILGenerator();
        il.Emit(OpCodes.Ret);

        return method.CreateDelegate<Action>();
    }
}

[MichalPetryka]

Seems like no noticeable regressions outside noise.

[jkotas]

EqualsTrue 0.34

Can we do better on this one by reordering the checks in Equals?

[jkotas]

Do you expect that real world code calls Equals often enough with the exact same instance to make this fast path worth it?

[MichalPetryka]

I'd assume most Equals usages to be with lambdas and event subscribes/unsubscribes so I'd assume most true cases to be with reference equality.

[jkotas]

I agree that most Equals usage is likely going to event unsubscribes.

I think event unsubscribes are typically done with instances methods - random example https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.notnetcoreapp.cs#L50-L51 - so this shortcut won't apply for them.

[jkotas]

This is not that perf critical, so the extra check is not a big deal. Looking at the overall Equals flow, it is trying to have multiple paths that are supposed to help. It is not clear whether they are (still) helping in practice.

[MichalPetryka]

This is not that perf critical, so the extra check is not a big deal. Looking at the overall Equals flow, it is trying to have multiple paths that are supposed to help. It is not clear whether they are (still) helping in practice.

I agree that it might be cheaper to remove some now that we can check MethodDesc more cheaply, but I think it'd be better to leave that for followup, especially if we'll want to make MethodDesc eagerly inited.

[MichalPetryka]

EqualsTrue 0.34

Can we do better on this one by reordering the checks in Equals?

I don't think we can with the multicast one, the unmanged one maybe but haven't fully thought it out.

The benchmark being 1ns is too short to measure accurately though so the value is likely within noise instead.

[MichalPetryka]

@EgorBot -amd -arm

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;
using System;
using System.Reflection;
using System.Linq;

public class Benchmarks
{
    public Action[] b1;
    public Action[] b2;

    [GlobalSetup]
    public void Setup()
    {
        b1 = Enumerable.Repeat(new Action(B), 1000).ToArray();
        b2 = Enumerable.Repeat(new Action(B), 1000).ToArray();
    }

    public void B() { }

    [Benchmark]
    public bool EqualsTrue()
    {
        var b1c = b1;
        var b2c = b2;
        for (int i = 0; i < b1c.Length; i++)
            if (b1c[i] != b2c[i])
                return false;
        return true;
    }
}