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: Activator.CreateFactory and ConstructorInfo.CreateDelegate #36194

Closed
Tracked by #75358
GrabYourPitchforks opened this issue May 11, 2020 · 30 comments
Closed
Tracked by #75358
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection Cost:M Work that requires one engineer up to 2 weeks
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented May 11, 2020

(See also #32520.)

API proposal

namespace System
{
    public static class Activator
    {
      // EXISTING APIs (abridged):
      public static T CreateInstance<T>();
      public static object? CreateInstance(Type type);
      public static object? CreateInstance(Type type, bool nonPublic);

      // NEW PROPOSED APIs:
      public static Func<T> CreateFactory<T>(); // no new() constraint, matches existing API
      public static Func<object?> CreateFactory(Type type);
      public static Func<object?> CreateFactory(Type type, bool nonPublic);
    }
}

// STRETCH GOAL APIs (see end of proposal):
namespace System.Reflection
{
    public class ConstructorInfo
    {
      public static TDelegate CreateDelegate<TDelegate>();
      public static Delegate CreateDelegate(Type delegateType);
    }
}

// REMOVED FROM PROPOSAL
/*
namespace System.Runtime.CompilerServices
{
    public static class RuntimeHelpers
    {
        // EXISTING API:
        public static object GetUninitializedObject(Type type);

        // NEW PROPOSED API:
        public static Func<object> GetUninitializedObjectFactory(Type type);
    }
}
*/

Discussion

Instantiating objects whose types are not known until runtime is fairly common practice among frameworks and libraries. Deserializers perform this task frequently. Web frameworks and DI frameworks might create per-request instances of objects, then destroy those objects at the end of the request.

APIs like Activator.CreateInstance and the System.Reflection surface area can help with this to a large extent. However, those are sometimes seen as heavyweight solutions. We've built some caching mechanisms into the framework to suppose these use cases. But it's still fairly common for high-performance frameworks to bypass Activator and the reflection stack and to go straight to manual codegen. See below for some examples.

See below for some concrete samples.

System.Text.Json

if (realMethod == null)
{
LocalBuilder local = generator.DeclareLocal(type);
generator.Emit(OpCodes.Ldloca_S, local);
generator.Emit(OpCodes.Initobj, type);
generator.Emit(OpCodes.Ldloc, local);
generator.Emit(OpCodes.Box, type);
}
else
{
generator.Emit(OpCodes.Newobj, realMethod);
}

ASP.NET Core

dotnet/aspnetcore#14615 (though they're using TypeBuilder to work around this right now)

Other runtime + libraries

// Implemet the static factory
// public object Create(IDictionary<string, object>)
// {
// return new <ProxyClass>(dictionary);
// }
MethodBuilder factoryMethodBuilder = proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName, MethodAttributes.Public | MethodAttributes.Static, typeof(object), CtorArgumentTypes);
ILGenerator factoryIL = factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj, proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Ref emit incurs a substantial upfront perf hit, but it does generally provide a win amortized over the lifetime of the cached method as it's invoked over and over again. However, this comes with its own set of problems. It's difficult for developers to get the exact IL correct across the myriad edge cases that might exist. It's not very memory-efficient. And as runtimes that don't allow codegen become more commonplace, it complicates the callers' code to have to decide the best course of action to take for any given runtime.

These proposed APIs attempt to solve the problem of creating a basic object factory using the best mechanism applicable to the current runtime. The exact mechanism used can vary based on runtime: perhaps it's codegen, perhaps it's reflection, perhaps it's something else. But the idea is that the performance of these APIs should rival the best hand-rolled implementations that library authors can create.

Shortcomings, not solved here

This API is not a panacea to address all performance concerns developers have with the reflection stack. For example, this won't change the perf characteristics of MethodInfo.Invoke. But it could be used to speed up the existing Activator.CreateInstance<T> APIs and to make other targeted improvements. In general, this API provides an alternative pattern that developers can use so that they don't have to roll solutions themselves.

It also does not fully address the concern of calls to parameterized ctors, such as you might find in DI systems. The API RuntimeHelpers.GetUninitializedObjectFactory does help with this to some extent. The caller can cache that factory to instantiate "blank" objects quickly, then use whatever existing fast mechanism they wish to call the object's real ctor over the newly allocated instance.

I hope to have a better solution for this specific scenario in a future issue.

Stretch goal APIs

The APIs ConstructorInfo.CreateDelegate<TDelegate>() and friends are meant to allow invoking parameterless or parameterful ctors. These are primarily useful when the objects you're constructing have a common signature in their constructors, but you don't know the actual type of the object at compile time.

Consider:

public class MyService
{
    public MyService(IServiceProvider serviceProvider) { }
}

public class MyOtherService
{
    public MyOtherService(IServiceProvider serviceProvider) { }
}

In these cases, the caller would get the ConstructorInfo they care about, then call constructorInfo.CreateDelegate<Func<IServiceProvider, object>>(). Depending on which ConstructorInfo was provided, the returned delegate will create either a MyService or a MyOtherService, calling the appropriate ctor with the caller-provided IServiceProvider.

I say "stretch goal" because the parameter shuffling involved here would involve spinning up the JIT. We can keep this overhead to a minimum because within the runtime we can take shortcuts that non-runtime components can't take with the typical DynamicMethod-based way of doing things. So while there's less JIT cost compared to a standard DynamicMethod-based solution, there's still some JIT cost, so it doesn't fully eliminate startup overhead. (This overhead isn't much different than the overhead the runtime already incurs when it sees code like Func<string, bool> func = string.IsNullOrEmpty;, since the runtime already needs to create a parameter shuffling thunk for this scenario.)

@GrabYourPitchforks GrabYourPitchforks added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection labels May 11, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 11, 2020
@davidfowl
Copy link
Member

Related

public static ObjectFactory CreateFactory(Type instanceType, Type[] argumentTypes)
. DI enabled, find me a constructor to create a factory for.

Here's the ObjectFactory definition

delegate object ObjectFactory(IServiceProvider serviceProvider, object[] arguments);
.

Example usage:
https://github.com/dotnet/aspnetcore/blob/04c3192d3cec396a9f2343fb5407d6777349b9df/src/SignalR/server/Core/src/Internal/DefaultHubActivator.cs#L13

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented May 11, 2020

Yeah, that's the part I'm having trouble with. What you're talking about is the more general case of fast method invocation. I'm blanking right now on a good, reliable, high-performance way to do that without involving codegen. Right now MethodInfo.Invoke is the best tool to suit that need due to how extremely flexible it is. But of course this flexibility comes at a price, and many callers don't want to pay the price for the flexibility they don't need. Aspnet is a good case study on this. :)

Let's assume for the moment that you had a good way of invoking arbitrary instance methods in a high-performance fashion. The APIs proposed here would allow you to extend that same concept to constructors of arbitrary signature. To do this, you'd use the proposed RuntimeHelpers.GetUninitializedObjectFactory API to get a zero-inited instance of the target object, where no ctors have been run. Then you'd invoke the ctor as you would any other instance method.

Edit: As an example of what I said above, assume that the pattern you wanted to support instantiating was .ctor(IServiceProvider). To do that on top of this API, your factory helper would look like the below.

public class MyFactory
{
    private Func<object> _allocator;
    private Action<object, IServiceProvider> _ctor;

    public MyFactory(Type targetType)
    {
        if (targetType.IsValueType) { /* throw */ }
        _allocator = RuntimeHelpers.GetUninitializedObjectFactory(targetType);
        MethodInfo mi = targetType.GetMethod(".ctor", new Type[] { typeof(IServiceProvider) });
        _ctor = (Action<object, IServiceProvider>)mi.CreateDelegate(typeof(Action<object, IServiceProvider>), null /* instance */);
    }

    public object CreateInstance(IServiceProvider serviceProvider)
    {
        object instance = _allocator(); // these two calls together are basically 'newobj'
        _ctor(instance, serviceProvider);
        return instance;
    }
}

In this scenario there's zero codegen involved. If you wanted to support value types it gets a bit more complicated. Maybe that's something we could help with here.

@stephentoub
Copy link
Member

stephentoub commented May 11, 2020

public static Func<T> CreateFactory<T>();

I'm trying to understand the benefit of this over Activator.CreateInstance<T>(). If the above yields significant improvements, why wouldn't Activator.CreateInstance<T>() be changed to the equivalent of:

private static class Cache<T> { public static readonly Func<T> Func = CreateFactory<T>(); }
public static T CreateInstance<T>() => Cache<T>.Func();

?

I'm sure I'm just missing something.

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented May 11, 2020

I do plan on adding some of the existing Activator code paths to take advantage of this. But there's still considerable indirection while the activator tries to look up the appropriate factory method. In my experimentation this indirection alone was twice the cost of instantiating the target object.

For applications which maintain their own caching mechanisms we can return to them a factory which bypasses all of the built in indirection. This is the most desirable from the perspective of giving them bare metal performance so that there's no need for them to use their own codegen.

Edit: activator doesn't even initialize the cache until the second call to the constructor. The reason is that we want to optimize for the case where the type is instantiated only once, perhaps as part of app startup.

@jkotas
Copy link
Member

jkotas commented May 11, 2020

CreateFactory(Type type, bool nonPublic)

Do we also need an overload without nonPublic argument?

(Also, we need to be thinking about the linker annotations when introducing new reflection APIs.)

I'm trying to understand the benefit of this over Activator.CreateInstance()

For the generic overload, the fundamental advantage of the factory method compared to Activator.CreateInstance<T> is that Activator.CreateInstance<T> wraps exceptions in TargetInvocationException that adds an extra frame with EH. The factory method avoids this overhead. To get rid of this overhead, we would need to do a breaking change to stop wrapping exceptions in TargetInvocationException.

For the non-generic overload, the additional fundamental advantage of the factory method compared to Activator.CreateInstance are the indirections from the caching policy. The factory method avoids this overhead by moving the caching policy to the application. To get rid of this overhead, we would need to do a non-pay-for-play caching policy that would make all System.RuntimeType instances more expensive.

@GrabYourPitchforks
Copy link
Member Author

Do we also need an overload without nonPublic argument?

We probably should, for convenience and to help lessen confusion.

Also, we need to be thinking about the linker annotations when introducing new reflection APIs.

Do you have pointers on this topic? I don't see any annotations on the existing Activator.CreateInstance API, so I guess the linker itself is aware of the Activator APIs and special-cases calls to them?

@jkotas
Copy link
Member

jkotas commented May 11, 2020

Do you have pointers on this topic?

#36229 . It is work in progress. These annotations were discussed during the API review meetings a few weeks ago.

@stephentoub
Copy link
Member

stephentoub commented May 12, 2020

Activator.CreateInstance wraps exceptions in TargetInvocationException that adds an extra frame with EH. The factory method avoids this overhead

I see. It's unfortunate we pay that overhead, in particular in the : new() constraint case, where the developer is just writing new T() and so a TargetInvocationException is even more unexpected. Would we recommend the C# compiler change its code gen to use the new APIs? Presumably not, for the same breaking change concern?

@jkotas
Copy link
Member

jkotas commented May 12, 2020

Presumably not, for the same breaking change concern?

Right. Second reason for not recommending this API to C# compiler is that Roslyn would need to generate a boilerplate to cache the delegate.

The factory method adds an extra frame as well compared to just calling new MyType(). It does not have EH, but it is still an extra frame. And the factory method pays an extra overhead for delegate invocation. I guess that the generic factory method may have no performance advantage compared to optimized Activator.CreateInstance<T>.

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented May 13, 2020

Using Activator.CreateInstance(Type, ...) is slightly faster than Activator.CreateInstance<T>() for reference types, but I think the strongly typed overload is still worth the convenience.

Activator.CreateInstance<T>() (where T is a value type with an explicit ctor) allocates then unboxes. Activator.CreateFactory<T>() is allocation free for this scenario. But we might be able to perform whatever contortions we need to behind the scenes to make CreateInstance allocation free here as well.

@stephentoub
Copy link
Member

stephentoub commented May 13, 2020

Using Activator.CreateInstance(Type, ...) is slightly faster than Activator.CreateInstance<T>() for reference types

Only in specific situations, such as using the generic overload from another generic context? Otherwise I don't see that.. I just tried this:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;

[MemoryDiagnoser]
public class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssemblies(new[] { typeof(Program).Assembly }).Run(args);

    [Benchmark] public Program A1() => Activator.CreateInstance<Program>();
    [Benchmark] public object A2() => Activator.CreateInstance(typeof(Program));
    [Benchmark] public Program A3() => (Program)Activator.CreateInstance(typeof(Program));
}

and got this:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
A1 28.89 ns 0.246 ns 0.205 ns 0.0038 - - 24 B
A2 30.03 ns 0.512 ns 0.479 ns 0.0038 - - 24 B
A3 30.17 ns 0.257 ns 0.241 ns 0.0038 - - 24 B

Activator.CreateInstance<T>() (where T is a value type with an explicit ctor) allocates then unboxes.

Since you can't currently write such a T in C#, I'm not very concerned about optimizing for that case. It also seems like something we should be able to fix if it was really meaningful.

@benaadams
Copy link
Member

Using Activator.CreateInstance(Type, ...) is slightly faster than Activator.CreateInstance<T>() for reference types, but I think the strongly typed overload is still worth the convenience.

In practice should the comparison be with (T)Activator.CreateInstance(typeof(T), ...); since you would likely immediately cast object to something usable

@jkotas
Copy link
Member

jkotas commented May 13, 2020

Using Activator.CreateInstance(Type, ...) is slightly faster than Activator.CreateInstance<T>() for reference types

That is because of the current implementation of Activator.CreateInstance<T>() in CoreCLR leaves a lot of performance on the table. We can make it 2x-3x faster if we ported the implementation strategy for it from CoreRT.

E.g. run this:

class Program
{
    static void Main(string[] args)
    {
        for (;;)
        {
            {
                Stopwatch sw = new Stopwatch();
                sw.Start();
                for (int i = 0; i < 100000000; i++)
                    GC.KeepAlive(Activator.CreateInstance<object>());
                Console.WriteLine($"CreateInstance<T> {sw.ElapsedMilliseconds}");
            }

            {
                Type objType = typeof(object);
                Stopwatch sw = new Stopwatch();
                sw.Start();
                for (int i = 0; i < 100000000; i++)
                    GC.KeepAlive(Activator.CreateInstance(objType));
                Console.WriteLine($"CreateInstance(Type) {sw.ElapsedMilliseconds}");
            }
        }
    }
}

CoreCLR:

CreateInstance<T> 4249
CreateInstance(Type) 4035

CoreRT:

CreateInstance<T> 1514
CreateInstance(Type) 25089 // CoreRT does not have the activator cache like CoreCLR

@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Jun 29, 2020
@GrabYourPitchforks GrabYourPitchforks added this to the Future milestone Jul 7, 2020
@GrabYourPitchforks GrabYourPitchforks changed the title API proposal: Activator.CreateFactory and RuntimeHelpers.GetUninitializedObjectFactory API proposal: Activator.CreateFactory Nov 19, 2020
@GrabYourPitchforks GrabYourPitchforks added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 19, 2020
@GrabYourPitchforks GrabYourPitchforks modified the milestones: Future, 6.0.0 Nov 19, 2020
@GrabYourPitchforks GrabYourPitchforks changed the title API proposal: Activator.CreateFactory API proposal: Activator.CreateFactory and ConstructorInfo.CreateDelegate Nov 24, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 24, 2020
@terrajobst
Copy link
Member

terrajobst commented Nov 24, 2020

Video

  • We don't add a new() constraint to match the existing API
namespace System
{
    public partial class Activator
    {
        // Existing APIs
        // public static T CreateInstance<T>();
        // public static object? CreateInstance(Type type);
        // public static object? CreateInstance(Type type, bool nonPublic);

        public static Func<T> CreateFactory<T>();
        public static Func<object?> CreateFactory(Type type);
        public static Func<object?> CreateFactory(Type type, bool nonPublic);
    }
}
namespace System.Reflection
{
    public class ConstructorInfo
    {
      public static TDelegate CreateDelegate<TDelegate>();
      public static Delegate CreateDelegate(Type delegateType);
    }
}

@GrabYourPitchforks
Copy link
Member Author

FWIW, we can play some games within ConstructorInfo.CreateDelegate to make it optimized beyond what normal consumers can do.

For example, say that you have these three call sites:

private object? MySharedDelegate(ref JsonSerializerOptions options);

Func<string, object> factory1 = ci1.CreateDelegate<Func<string, object>>();
Func<IServiceProvider, IService> factory2 = ci2.CreateDelegate<Func<IServiceProvider, IService>>();
MySharedDelegate factory3 = (MySharedDelegate)ci3.CreateDelegate(typeof(MySharedDelegate));

We'd only need to spin up the JIT once to handle all of these cases, as our parameter shuffling thunk can be shared between all factory signatures. Normal ref emit-based consumers can't make this same optimization.

@GrabYourPitchforks
Copy link
Member Author

Getting System.Text.Json to use this new API would benefit from #4556 (comment) being resolved. The APIs proposed here return Func<object?>, but System.Text.Json has its own signature-compatible delegate type that it uses everywhere. Working around the issue is possible but not desirable from a code cleanliness / maintainability perspective.

@jkotas
Copy link
Member

jkotas commented Dec 1, 2020

System.Text.Json has its own signature-compatible delegate type that it uses everywhere

Is it used in the public surface or just internally?

@jkotas
Copy link
Member

jkotas commented Dec 1, 2020

FWIW, .NET design guidelines suggest to use Action/Func as much as possible. If we do not like Action/Func from a code cleanliness / maintainability perspective, it sounds like a problem in the design guidelines.

@GrabYourPitchforks
Copy link
Member Author

IIRC those guidelines only affect the public API surface, not any internal usage. And honestly I'm not a huge fan of them, as after the Katana experiment and dealing with Func<string, int, Action<object, ...>, ...> everywhere you begin to long for delegate types where the delegate type names have meaning and the invoke method parameters have actual names.

@jkotas
Copy link
Member

jkotas commented Dec 1, 2020

Right, the guidelines talk about public APIs, but they indirectly affect what natural internal implementations looks like in many cases. Using different guidelines for internal implementations would often be inconsistent and add unnecessary overhead from back-and-forth conversions.

You can reduce the burden of repeating the full signature everywhere via using ConstructorDelegate = System.Func<object>; at the top of the file. I know that it is not perfect since you have to do it in every file today.

@krwq
Copy link
Member

krwq commented Jul 8, 2021

@GrabYourPitchforks feel free to change milestone back if you're planning to do this in this release

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 13, 2022

Doesn't seem we could make it within 7.0

@steveharter
Copy link
Member

I think it makes sense to close this issue.

For v8, we added ConstructorInvoker and MethodInvoker classes which are essentially a user-created factory class: #88415 where the implementation is free to choose an interpreted path, emit path, or fast non-emit path although the fast non-emit path only exists for NativeAOT via emit stubs.

These new APIs also support fixed-parameter arguments to avoid the object[] allocation with <= 4 parameters, and also support Span-based overloads that allow for other optimizations to avoid allocs with 5+ parameters and\or to use pooling of object[].

However, they are based on emit once called a second time so they are not code-less except as mentioned above for NativeAOT.

Also see #78917 which proposes having existing reflection APIs fall back to Activator for the fast cases which do not require emit.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 1, 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 Cost:M Work that requires one engineer up to 2 weeks
Projects
No open projects
Development

No branches or pull requests

10 participants