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

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, new(), 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.

---

Tasks:

• Refactor delegates to be immutable (see below)
• Build a functional-enough prototype
• 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.