Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Add invoker classes for fast invoke APIs #85539

Closed
steveharter opened this issue Apr 28, 2023 · 16 comments · Fixed by #88415
Closed

[API Proposal]: Add invoker classes for fast invoke APIs #85539

steveharter opened this issue Apr 28, 2023 · 16 comments · Fixed by #88415
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection tenet-performance Performance related issue
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Apr 28, 2023

Background and motivation

This will add "fast invoke" capability, allowing for up to 2x CPU improvement and eliminating the object[] parameters alloc for some scenarios. The existing MethodBase.Invoke(...) will also leverage this when possible. It is part of a larger goal to reduce usage of Reflection.Emit where feasible, which means performance is critical since Emit was originally used over Invoke because of performance.

API Proposal

New types:

namespace System.Reflection
{
    public abstract class MethodBaseInvoker
    { 
        internal MethodBaseInvoker(); // Or any internal ctor that removes the default ctor preventing external derivations.
    }

    public sealed class ConstructorInvoker : MethodBaseInvoker
    {
        // The caller is assumed to cache the invoker.
        public static ConstructorInvoker Create(ConstructorInfo constructor);

        internal ConstructorInvoker(); // Or any internal ctor that removes the default ctor preventing default constructor.

        // All methods have the same semantics as 'MethodBase.Invoke(...)' for validation and defaulting with the
        // possible exception (TBD) of supporting 'Type.Missing'; the user would need to use 'ParameterInfo.DefaultValue'.

        // This supports copy-back for ref\out args like 'MethodBase.Invoke(...)'.
        public object? Invoke(Span<object?> arguments);

        public object? Invoke();

        // These cannot support copy-back; adding 'ref' modifier is not desired here.
        public object? Invoke(object? arg1);
        public object? Invoke(object? arg1, object? arg2);
        public object? Invoke(object? arg1, object? arg2, object? arg3);
        public object? Invoke(object? arg1, object? arg2, object? arg3, object? arg4);
    }

    public sealed class MethodInvoker : MethodBaseInvoker
    {
        public static MethodInvoker Create(MethodInfo method);

        internal MethodInvoker(); // Or any internal ctor that removes the default ctor preventing default constructor.

        public object? Invoke(object? obj, Span<object?> arguments);
        public object? Invoke(object? obj);
        public object? Invoke(object? obj, object? arg1);
        public object? Invoke(object? obj, object? arg1, object? arg2);
        public object? Invoke(object? obj, object? arg1, object? arg2, object? arg3);
        public object? Invoke(object? obj, object? arg1, object? arg2, object? arg3, object? arg4);
    }
}

API Usage

Simple replacement for MethodBase.Invoke:

MethodInfo method = typeof(TestClass).GetMethod(nameof(TestClass.Method1))!;
MethodInvoker cachedInvoker = MethodInvoker.Create(method);

TestClass obj = new();

// Invoke the method without having to create an `object[]` to contain the args.
cachedInvoker.Invoke(obj, 42);

public class TestClass
{
    public void Method1(int i) { }
}

For high-performance APIs such as DI's ActivatorUtiliities.CreateFactory():

private static object CallConstructor(ConstructorInvoker cachedInvoker, Span<object> args)
{
    // 'args' is not the exact set of parameters; we have "Transform()" logic to determine
    if (args.Length <= 4)
    {
        // Fast logic to avoid object[] allocation
        switch (args.Length)
        {
            case 0:
                return cachedInvoker.Invoke();
            case 1:
                return cachedInvoker.Invoke(args[0].Transform());
            case 2:
                return cachedInvoker.Invoke(args[0].Transform(), args[1].Transform());
            case 3:
                return cachedInvoker.Invoke(args[0].Transform(), args[1].Transform(), args[2].Transform());
            case 4:
                return cachedInvoker.Invoke(args[0].Transform(), args[1].Transform(), args[2].Transform(), args[3].Transform());
        }
    }
    else
    {
        Span<object> copyOfArgs = new object[args.Length];
        for (int i = 0; i < args.Length; i++)
        {
            copyOfArgs[i] = args[i].Transform();
        }
        return cachedInvoker.Invoke(copyOfArgs);
    }
}

private static object Transform(this object obj)
{
    object newObj = // Assume special logic here such as defaulting or conversion of parameters.
    return newObj;
}

Validation

Alternative Designs

Note that these proposed invoker classes already exist internally.

Support for byref-like types is covered here which has been prototyped successfully. However, the performance of that is not ideal when only System.Object is necessary which is the case for DI's CreateFactory which calls constructors with user-specified object - based args.

@steveharter steveharter added api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection labels Apr 28, 2023
@steveharter steveharter added this to the 8.0.0 milestone Apr 28, 2023
@ghost
Copy link

ghost commented Apr 28, 2023

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

Issue Details

Background and motivation

(placeholder)

API Proposal

(placeholder)

API Usage

(placeholder)

Alternative Designs

(placeholder)

Risks

(placeholder)

Author: steveharter
Assignees: -
Labels:

api-needs-work, area-System.Reflection

Milestone: 8.0.0

@jkotas
Copy link
Member

jkotas commented Apr 28, 2023

  • The factory method can be on the MethodInvoker/ConstructorInvoke methods. There is no reasonable way for non-runtime MethodInfos to provide implementation for this since the Invoker types are sealed.
  • We may want to have separate types for invoking construtors vs. invoking methods. It would allow us to avoid some overhead in the AOT scenarios:
public class MethodInvoker
{
        public ConstructorInvoker Create(MethodBase info);
...
}

public class ConstructorInvoker
{
        public ConstructorInvoker Create(ConstructorInfo info);
...
        // We do not need this ptr for these.
        public object? Invoke(Span<object?> arguments);

        public object? Invoke(object? arg1);
        public object? Invoke(object? arg1, object? arg2);
        public object? Invoke(object? arg1, object? arg2, object? arg3);
        public object? Invoke(object? arg1, object? arg2, object? arg3, object? arg4);
}

@teo-tsirpanis
Copy link
Contributor

How about adding extension methods to MethodInfo and ConstructorInfo instead of dedicated invoker classes?

@jkotas
Copy link
Member

jkotas commented Apr 30, 2023

How about adding extension methods to MethodInfo and ConstructorInfo instead of dedicated invoker classes?

The extension methods would have to first check for null "this" and then cast the MethodInfo to RuntimeMethodInfo. It is unnecessary overhead. The only reason why we are adding this is to squeeze performance out of reflection invoke.

@Joe4evr
Copy link
Contributor

Joe4evr commented May 1, 2023

public sealed class ConstructorInvoker
{
    public static ConstructorInvoker Create(this ConstructorInfo constructor);
}

Unless I'm unaware of a recent language change, extension methods can only exist in static classes, so this won't work.

@teo-tsirpanis
Copy link
Contributor

Guess it's a typo, an extension method for Create wouldn't make sense.

@steveharter steveharter added tenet-performance Performance related issue api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels May 1, 2023
@steveharter steveharter changed the title [API Proposal]: add MethodInvoker for fast invoke APIs [API Proposal]: Add invoker classes for fast invoke APIs May 2, 2023
@steveharter steveharter added the blocking Marks issues that we want to fast track in order to unblock other important work label May 4, 2023
@bartonjs
Copy link
Member

bartonjs commented May 9, 2023

Video

  • There was a question of why the Invoke methods took a Span instead of ReadOnlySpan. The answer was for ref/out semantics.
  • The base class is an implementation detail that shouldn't leak into the ref. It should be removed.
  • For MethodInvoker.Invoke, we checked that obj is the consistent name for the target/instance parameter.
  • We discussed the number of overloads/number of argN options, and felt that 3 or 4 are both OK (but that we really want params Span)
  • If there are places in reflection that use arg0..argN instead of arg1..argN, we should change the indexing here to match.
  • Create vs .ctor came up, and Create seems justified (e.g. future caching opportunities)
namespace System.Reflection
{
    public sealed class ConstructorInvoker
    {
        public static ConstructorInvoker Create(ConstructorInfo constructor);

        private ConstructorInvoker();

        public object? Invoke();
        public object? Invoke(object? arg1);
        public object? Invoke(object? arg1, object? arg2);
        public object? Invoke(object? arg1, object? arg2, object? arg3);
        public object? Invoke(object? arg1, object? arg2, object? arg3, object? arg4);
        public object? Invoke(Span<object?> arguments);
    }

    public sealed class MethodInvoker
    {
        public static MethodInvoker Create(MethodInfo method);

        private MethodInvoker();

        public object? Invoke(object? obj);
        public object? Invoke(object? obj, object? arg1);
        public object? Invoke(object? obj, object? arg1, object? arg2);
        public object? Invoke(object? obj, object? arg1, object? arg2, object? arg3);
        public object? Invoke(object? obj, object? arg1, object? arg2, object? arg3, object? arg4);
        public object? Invoke(object? obj, Span<object?> arguments);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 9, 2023
@jkotas
Copy link
Member

jkotas commented May 9, 2023

public sealed class MethodInvoker
public static MethodInvoker Create(MethodInfo method);

Nit: We may want to make this MethodBase so that it can be used to call constructors on existing instances (e.g. instance allocated using RuntimeHelpers.GetUninitializedObject).

@davidfowl
Copy link
Member

davidfowl commented May 10, 2023

We should try replacing our method info invocation with this and test the performance.

cc @halter73 @BrennanConroy @captainsafia

@steveharter steveharter self-assigned this May 30, 2023
@Neme12
Copy link

Neme12 commented Jul 3, 2023

Why not make the Invoke methods generic so that people can avoid boxing?

        public TResult Invoke<T1, TResult>(T1 arg1);
        public TResult Invoke<T1, T2, TResult>(T1 arg1, T2 arg2);
        public TResult Invoke<T1, T2, T3, TResult>(T1 arg1, T2 arg2, T3 arg3);
        public TResult Invoke<T1, T2, T3, T4, TResult>(T1 arg1, T2 arg2, T3 arg3, T4 arg4);
        // and i would add more... until 8 or 16 so that people can avoid having to use the boxing Span overload

@Neme12
Copy link

Neme12 commented Jul 3, 2023

Actually, I think it would be a lot better and a lot more user friendly to make the class itself generic, so that when it's instantiated, it already has the right Invoke method with the right type parameters and the user can't accidentally use different ones.

That would require a lot more invoker classes, though, e.g.:

public sealed class MethodInvoker<TObj, TResult>
{
    public TResult Invoke(TObj obj);
}

public sealed class MethodInvoker<TObj, TArg1, TResult>
{
    public TResult Invoke(TObj obj, TArg1 arg);
}

public sealed class MethodInvoker<TObj, TArg1, TArg2, TResult>
{
    public TResult Invoke(TObj obj, TArg1 arg, TArg2);
}

// ...etc

It would be a lot more user friendly though and more importantly, avoid any boxing.

Or I'm thinking there might be a way to still only have one class by having the type parameter be a delegate:

public static class MethodInvoker
{
    public static MethodInvoker<TDelegate> Create<TDelegate>(MethodInfo method) where TDelegate : Delegate;
}

public sealed class MethodInvoker<TDelegate> where TDelegate : Delegate
{
    public TDelegate Invoke { get; }
}

Usage would look like this:

var invoker = MethodInvoker.Create<Func<int, string>>(myMethod);
string result = invoker.Invoke(5);

although this second approach might add a bit of overhead by having to allocate the delegate. But that would only be a one-time cost when creating the invoker and invoking the method would still avoid any boxing. And the implementation could actually be even more efficient because it could use reflection emit to prepare the invoke delegate since it already has the right parameter count and parameter types. If that were the case, this would be the most efficient way to dynamically invoke a method as it would have the same perf as manually using reflection emit. People who use reflection emit to invoke a method today could delete all that code and instead use this because it would have the same exact perf.

I see no point in the API as currently proposed because it still boxes and it still wouldn't be the fastest way to invoke a method. The approach I propose would be a) more user friendly by being strongly typed and b) faster as well - as fast as it can get.

@Joe4evr
Copy link
Contributor

Joe4evr commented Jul 3, 2023

Why not make the Invoke methods generic so that people can avoid boxing?

Most often when reflection is used, it's to deal with types you don't know, so you can't provide an appropriate type argument.

@Neme12
Copy link

Neme12 commented Jul 3, 2023

Interesting, I guess I use reflection differently. IIRC, any time I've used reflection invoke APIs, I always had strongly typed values and having to use an object array was just an ugly necessary evil. At least we could have both options. the Span<object> one for when you don't have strongly typed values and the faster strongly typed one for when you do.

EDIT: Never mind. I had no idea I could just use Delegate.CreateDelegate in all these cases instead of reflection. 🤦

@koszeggy
Copy link

koszeggy commented Jul 3, 2023

Oh wow, it seems I can finally retire my method invokers in .NET8+.

I can't wait for trying it out.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 15, 2023
@steveharter
Copy link
Member Author

Note that ConstructorInvoker.Invoke() return value should not be nullable; this will be fixed.

@davidfowl
Copy link
Member

@steveharter are you planning to update DI to use the ConstructorInvoker?

@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants