Optimize and simplify delegate layout

[MichalPetryka]

Reduce size of delegate instance by one pointer field

Contributes to #85014.

[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.)

[jkotas]

/__w/1/s/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs(336,25): error CS0128: A local variable or function named 'method' is already defined in this scope [/__w/1/s/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj]
/__w/1/s/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs(340,20): error CS0266: Cannot implicitly convert type 'System.IRuntimeMethodInfo' to 'System.Reflection.MethodInfo'. An explicit conversion exists (are you missing a cast?) [/__w/1/s/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj]
/__w/1/s/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs(336,25): error IDE0059: Unnecessary assignment of a value to 'method' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0059) [/__w/1/s/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj]

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/tests/UnitTests/ObjectTests.cs:450

• GetDelegateInfo now has explicit multicast detection via Delegate.HelperObject being an array, but the unit tests no longer exercise that path (the previous "Multicast" test was repurposed to "Unmanaged"). Please add a multicast test that sets HelperObject to an array object and verifies DelegateType.Unknown (and zeroed Target/MethodPtr) for multicast delegates.

    [Theory]
    [ClassData(typeof(MockTarget.StdArch))]
    public void GetDelegateInfo_Unmanaged(MockTarget.Architecture arch)
    {
        const ulong TestMethodTable = 0x00000000_10000200;
        const ulong TestMethodPtr = 0x00000000_aaaa0000;
        const long TestExtraData = -1;
        TargetPointer delegateAddress = default;

        IObject contract = CreateObjectContract(
            arch,
            objectBuilder =>
            {
                delegateAddress = objectBuilder.AddDelegateObject(
                    methodTable: TestMethodTable,
                    target: 0,
                    methodPtr: TestMethodPtr,
                    methodPtrAux: 0,
                    extraData: TestExtraData);
            });

        DelegateInfo info = contract.GetDelegateInfo(delegateAddress);

        Assert.Equal(DelegateType.Unknown, info.DelegateType);
        Assert.Equal(0ul, info.TargetObject.Value);
        Assert.Equal(0ul, info.TargetMethodPtr.Value);
    }

[jkotas]

LGTM once the remaining feedback is addressed

[MichalPetryka]

LGTM once the remaining feedback is addressed

I'll get to it tomorrow

[MichalPetryka]

@jkotas I think everything should be ready, I decided to go with IsClosed after looking at all the places and it seemed cleaner to me everywhere but I can revert it if you don't like it.

[jkotas]

/ba-g infrastructure timeouts

[jkotas]

Thank you! There are two follow up changes that I would like to see (two separate PRs):

• Move everything from MulticastDelegate.CoreCLR.cs to Delegate.CoreCLR.cs
• Change object[] to Wrapper[] like what it is in NativeAOT to avoid casts and covariance checks
  Do you plan to work on these?

[jkotas]

Move everything from MulticastDelegate.CoreCLR.cs to Delegate.CoreCLR.cs

Actually, let me do this one - it is trivial and it is going to fix app compat break #130612