[API Proposal]: Introduce an intrinsic for more efficient lambda generation

[MichalPetryka]

Background and motivation

Roslyn currently caches lambda delegates in static mutable fields in nested classes and initializes them lazily at the use site (guarded by a null check).
This pattern is inefficient for current runtimes, being hard to detect and optimize correctly, leading to dead code being left in generated assembly and making optimizations like delegate inlining more tricky.
Discussion here ended up with two possible solutions: creating a runtime intrinsic that'd create and cache the delegate for Roslyn or caching the delegates in static readonly fields which CoreCLR and Native AOT can analyze easily even today but that'd come with some metadata cost.

The API can create delegates closed over null for most cases, putting them on the Frozen Object Heap.
For delegates to instance methods on generic types, the runtime would be required to create an instance instead. Such support would make the implementation relatively more complicated, as such it's not decided yet whether the API should work then or throw. If it is to work, the API should forbid the owning type from having a cctor and a finalizer due to only creating instances in some cases and leaving them uninitialized.
The API can't always return the same instance due to not being able to always expand as intrinsic for shared generics, the instance count will however always be small and they'll always be value equal to each other.

In AOT scenarios, the API requires to match a specific IL pattern recognized by the runtime to work for non reflection visible methods.

cc @jkotas @EgorBo @jaredpar @CyrusNajmabadi

API Proposal

From #125901:

public class RuntimeHelpers
{
    [Intrinsic]
    public static TDelegate GetDelegate<TDelegate>(nint method) where TDelegate : Delegate;
}

API Usage

This would be used internally by Roslyn when emitting lambdas.

Alternative Designs

Have Roslyn store delegates in static readonly fields - would work with CoreCLR and Native AOT, not require any runtime changes, but it'd introduce either more metadata bloat or more eager delegate object allocation (we'd either need a separate nested class per lambda or calling one lambda would then create delegates for all the lambdas in the nested class).

Risks

All runtimes including Mono would need to have this implemented.
It's difficult to guarantee the same instance will be always returned on all runtimes.

---

Tasks:

• Refactor delegates to be immutable (see below) have GC fields be immutable: Replace Delegate MethodInfo cache with the MethodDesc #99200
• Build a functional-enough prototype: [WIP] Implement an intrinsic for delegate lambdas #125901
• Submit an API proposal for review, incorporating learnings from the prototype

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

Issue Details

---

Background and motivation

Roslyn currently caches lambda delegates in static mutable fields in nested classes and initializes them lazily at the use site (guarded by a null check).
This pattern is inefficient for current runtimes, being hard to detect and optimize correctly, leading to dead code being left in generated assembly and making optimizations like delegate inlining more tricky.
Discussion here ended up with two possible solutions: creating a runtime intrinsic that'd create and cache the delegate for Roslyn or caching the delegates in static readonly fields which CoreCLR and Native AOT can analyze easily even today but that'd come with some metadata cost.

cc @jkotas @EgorBo @jaredpar @CyrusNajmabadi

API Proposal

From @jkotas in dotnet/csharplang#6746 (reply in thread):

public class RuntimeHelpers
{
    [Intrinsic]
    // ldftn instruction must immediately precede call of this method. The target method must
    // match the delegate signature and it must be an instance method on the TScope reference. TScope should
    // have no instance fields. Given function pointer can be only mapped to one delegate type using this method.
    static TDelegate GetLambdaSingleton<TScope, TDelegate>(IntPtr ftn) where TScope: class, TDelegate: delegate
    {
        // This is mock implementation. This method is always going to be expanded as JIT intrinsic.
        lock (s_table)
        {
            MethodInfo mi = MapEntryPointToMethodInfo(typeof(TScope), ftn);

            if (s_table.TryGetValue(mi, out Delegate del))
                return (TDelegate)del;

            Delegate ret = typeof(TScope).IsCollectible ? 
                 InternalCreateDelegate(typeof(TDelegate), new TScope(), ftn) :
                 InternalCreateFrozenDelegate(typeof(TDelegate), new-frozen TScope(), ftn);
            s_table.Add(mi, ret);
            return (TDelegate)ret;
        }
    }

    static readonly ConditionalWeakTable<MethodInfo, Delegate> s_table;
}

API Usage

This would be used internally by Roslyn when emitting lambdas.

Alternative Designs

Have Roslyn store delegates in static readonly fields - would work with CoreCLR and Native AOT, not require any runtime changes, but it'd introduce either more metadata bloat or more eager delegate object allocation (we'd either need a separate nested class per lambda or calling one lambda would then create delegates for all the lambdas in the nested class).

Risks

All runtimes including Mono would need to have this implemented.

Author:	MichalPetryka
Assignees:	-
Labels:	api-suggestion, area-System.Runtime.CompilerServices, untriaged
Milestone:	-

[jaredpar]

static TDelegate GetLambdaSingleton<TScope, TDelegate>(IntPtr ftn) where TScope: class,

Needs to include the new() constraint.

[jaredpar]

This will require a bit of refactoring on Roslyn's part. Today our emit strategy puts all lambdas for a given scope into the same closure type. That means it mixes capturing and non-capturing lambdas. In order to meet the constraints of this API we'll need to break up our closures such that non-capturing lambdas are never mixed with capturing ones.

I think this can be done by having a single closure per method for all of our non-capturing lambdas vs. having up to two per scope. Hopefully that is a bit cheaper given our implementation.

[EgorBo]

Have Roslyn store delegates in static readonly fields - would work with CoreCLR and Native AOT, not require any runtime changes

The plan is to allocate them on a frozen (nongc) segment which is not supported for static readonly in JIT today (but it can be done, just wanted to point out that there will be some runtime(jit) work any way).

[MichalPetryka]

Today our emit strategy puts all lambdas for a given scope into the same closure type.

It doesn't, it emits a type for all methods from the type.

[jaredpar]

It doesn't, it emits a type for all methods from the type.

My bad. It does look like we implemented that optimization already. I thought we had a number of cases where we still combined them with the capturing lambdas but does not appear so.

[MichalPetryka]

This should also be used for creation of delegates from method groups.

[MichalPetryka]

@jkotas @EgorBo While working on #85349, I've accidentally made delegate creation be regarded as slightly cheaper for the inliner, which triggers huge diffs for all the lambdas with the mostly dead initialization branch. This means that changing this will also trigger huge diffs due to removal of that branch.