Support basic MethodInfo caching pattern with MakeGenericMethod (and MakeGenericType)

[roji]

Following #2480, the linker identifies uses of MakeGenericMethod/Type where the type is statically. However, in order for the code to be understandable to the linker, it must perform reflection each time as follows:

var method = typeof(Program).GetMethod("Foo").MakeGenericMethod(typeof(Bar));
method.Invoke(null, null);

However, it's common to perform the reflection lookup once, and store the result in a static readonly variable. This is important for proper factoring where the same generic method is used frequently (e.g. EF Core), and can maybe also impact perf (through repeated calls to GetMethod):

private static readonly MethodInfo FooMethod = typeof(Program).GetMethod("Foo");

...

var method = FooMethod.MakeGenericMethod(typeof(Bar));
method.Invoke(null, null);

When doing this, DynamicallyAccessedMembers on FooMethod aren't taken into account, and the program fails at runtime (see full sample below). It would be helpful if the linker identified basic caching patterns such as the above to make this work.

Full code sample

class Program
{
    private static readonly MethodInfo FooMethod = typeof(Program).GetMethod("Foo");

    public static void Main()
        => Go(typeof(Program));

    public static void Go(Type type)
    {
        // Doesn't work - makes sense:
        // var method = type.GetMethod("Foo").MakeGenericMethod(typeof(Bar));
        // method.Invoke(null, null);

        // Works:
        // var method = typeof(Program).GetMethod("Foo").MakeGenericMethod(typeof(Bar));
        // method.Invoke(null, null);

        // Doesn't work - making this work is the issue ask:
        // var method = FooMethod.MakeGenericMethod(typeof(Bar));
        // method.Invoke(null, null);

        // Also works:
        // Foo<Bar>();
    }

    public static T Foo<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>()
    {
        Console.WriteLine("in Foo");
        return Activator.CreateInstance<T>();
    }

    class Bar
    {
        public void SomeMethod()
            => Console.WriteLine("In Bar.SomeMethod");
    }
}

[vitek-karas]

Thanks for the suggestion. This will be tricky though. For one, linker currently has no existing infrastructure to "remember" values for fields.

There's a possible workaround, but it's ugly. In the above sample use code like this:

    var fooMethod = FooMethod;
    if (FooMethod == null) {
        fooMethod = typeof(Program).GetMethod("Foo").MakeGenericMethod(typeof(Bar));
    }

    var method = fooMethod.MakeGenericMethod(typeof(Bar));
    method.Invoke(null, null);

This should work since linker won't (at least currently) recognize that FooMethod can never be null and it will use the if branch . It might still produce a warning, but at least it will correctly mark things (as in apply the All annotation to Bar).
Also, this has to be in the same method body where the MakeGenericMethod is called, for same reasons as above - linker currently doesn't have any capability to perform dataflow across method bodies.

[vitek-karas]

Another workaround:

private static readonly MethodInfo FooMethod = typeof(Program).GetMethod("Foo");
[UnconditionalSuppressMessage(...)]
private static MethodInfo InstantiateFooGeneric([DAM(All)] Type T)
=> FooMethod.MakeGenericMethod(T);

var method = InstantiateFooGeneric(typeof(Bar));
method.Invoke(null, null);

More code and the developer has to make sure to keep the annotations of the generic T and the parameter T in sync - which is not very nice 😢 .

[roji]

Yeah, the 2nd is what I'm currently doing.

I'd do the first workaround, but the objective is also to arrive at a zero-warning state - so if a warning is still emitted I'd still want to suppress that. There's also the issue that the actual reflection lookup is duplicated - once when initializing FooMethod, and once in the workaround.

This will be tricky though. For one, linker currently has no existing infrastructure to "remember" values for fields.

I imagined this would be the case. This just seems like a very common pattern in reflection-heavy applications, that some sort of special support for static readonly fields may be justified (assuming that's feasible).

[vitek-karas]

This just seems like a very common pattern in reflection-heavy applications, that some sort of special support for static readonly fields may be justified (assuming that's feasible).

I agree - and I think for readonly fields this could be workable - they never change value, and there should realistically only be one value assigned to them. This avoid lot of the complexity which would otherwise come from trying to track values across method calls. We will still have to solve ordering in the linker though (what if the linker sees the code which reads the field first, it will have to postpone or somehow pull in the init code for it before moving forward).

Another option is to come up with some kind of attribute similar to DynamicallyAccessedMembers which could be used on the field to track the necessary information across. Note that the linker doesn't need to know which exact method it is, it just needs to know annotations on the generic parameters. Having the annotation would make it a bit more work for the developer, but it might be useful in other scenarios - technically that might work even for non-read-only fields, or for methods which return MethodInfo and so on.

[roji]

Great, thanks for giving this thought.

FWIW I think the vast majority of cases fall under the static readonly field case, where a program simply needs to make generic methods out of some built-in open MethodInfo. At least in EF Core I'm not sure the more advanced variant would be useful (plus as you say it requires more annotating from the developer).

[MichalStrehovsky]

MakeGenericMethod does a lot more work than typeof followed by GetMethod. I don't think caching the MethodInfo makes a meaningful difference in the E2E scenario of "I need to instantiate this method" (I expect single-digit-percent throughput savings from caching, which should always be weighted against introducing a static constructor and an extra static base for the GC to scan).

typeof in CoreCLR is faster than accessing a static field. GetMethod goes directly to a name based cache that unless one has a lot of methods, finds things very fast.

I understand the argument for code cleanliness, but I don't think we should motivate the trimming feature to support this by perf.

[roji]

@MichalStrehovsky I agree - it's mainly a factoring question. When approaching a reflection-heavy codebase (EF), the effort of working around all those static readonly fields is considerable.

[kant2002]

I'm curious on what changed after some time passed and given that things improving overall. Since this issue affects at least 4 important codebases like ASP.NET Core, EF Core, F# and M.E.Logging

[vitek-karas]

Nothing really changed as far as I know. @kant2002 if you can point me to the patterns in question that would definitely help us figure out if there's something we can do to make this easier.

Currently I think the most tricky bit is if/how can we detect the value for a static readonly field and try thing around it. But in general this would require:

• Ability to correctly detect static readonly field value from a .cctor method body (data flow + some way to track static fields + some guard against multiple values)
• Ability to run data flow on .cctor before any other method on a given type. And how would we synchronize it with the rest of the linker
• Global store for readonly static field values

But first we should figure out if this is really worth the work.

[kant2002]

I record my observations about libraries which gives the most headaches during AOT scenarios. https://gist.github.com/kant2002/2a2585ef825c74445fd846a5f0499449

I cannot say this is best write-up, but I did try to collect patterns from some codebases which I think has something similar. I see them as the approximately the same pattern which is purely expressible in C#/F# and in IL probably. Or maybe this pattern is expressible in modern C#, but we are (as community) not there yet know how to do that.

[kant2002]

Nothing really changed as far as I know

thanks. Really just want to understand better if there something which I miss (very easy), and which improve things here.