Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Improve performance of Activator.CreateInstance - #32520

Merged
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance
Nov 26, 2020
Merged

Improve performance of Activator.CreateInstance#32520
GrabYourPitchforks merged 75 commits into
dotnet:masterfrom
GrabYourPitchforks:fast_createinstance

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
Member

Edit:ObjectFactory and UninitializedObjectFactory scrapped. This PR introduces no new API. See discussion in thread.


Contributes to #23716.

This is a very rough prototype of ObjectFactory and UninitializedObjectFactory, which allow fast codegen-free instantiation of arbitrary objects. It's primarily suited for DI and other scenarios where developers need to instantiate objects whose types aren't known until runtime but are required to do so with as minimal overhead as possible.

Historically, the way to instantiate such objects is to call Activator.CreateInstance or ConstructorInfo.Invoke. However, there is some overhead to this, as checks need to take place on each reflection API invocation. Some developers who need to eke out extra performance end up creating a DynamicMethod and hand-rolling a newobj instruction into the IL stream. This provides generally acceptable results, but it is tricky to get right, there's significant per-instantiation overhead due to spinning up the JIT, it's complex to debug issues in the code gen, and it doesn't work properly in environments where code gen is unavailable.

Note: These APIs have not yet been approved, as this is just an experimental PR to gauge general interest and to solicit feedback on the design and functionality.

General philosophy

Historically, most reflection APIs like Activator.CreateInstance and ConstructorInfo.Invoke perform checks on each method invocation. We can try to reduce the cost of these checks, but due to the nature of these APIs we'll never be able to remove the checks entirely.

The UninitializedObjectFactory and ObjectFactory APIs, on the other hand, perform all of their checks at construction time, not at invocation time. This means that it may be somewhat expensive to create the factory / dispenser itself, but once the factory is created it's very cheap to request a new instance from the factory. The intent is that these factories aren't used for one-off scenarios - callers should continue to use Activator.CreateInstance and ConstructorInfo.Invoke in such code paths. But in scenarios where the same type of object will be instantiated over and over again, it's very efficient to cache a single instance of the factory per-type and to query the cached instance whenever a new T instance is needed.

UninitializedObjectFactory

The UninitializedObjectFactory type is roughly equivalent to RuntimeHelpers.GetUninitializedObject. This instantiates the type without running any instance constructors. (Static constructors are still run.) Most applications should never need this, but it is useful for library authors building their own serialization or DI frameworks on top of this. The general pattern such library authors should follow is to use UninitializedObjectFactory to create a not-yet-constructed instance of target type T, then manually invoke the appropriate constructor on the newly-allocated instance.

ObjectFactory

The ObjectFactory type is roughly equivalent to Activator.CreateInstance<T>(). It allocates the type then calls the public, parameterless instance constructor. There are two main differences between ObjectFactory.CreateInstance and Activator.CreateInstance<T>():

  • If object construction fails, ObjectFactory.CreateInstance will not wrap the thrown exception within a TargetInvocationException. Activator.CreateInstance<T>() will wrap exceptions.
  • If T is a value type, ObjectFactory.CreateInstance will always return default(T). Activator.CreateInstance<T>() will call T's parameterless constructor if one exists; otherwise it returns default(T).

Potential consumers

System.Text.Json

if(realMethod==null)
{
LocalBuilderlocal=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);
// }
MethodBuilderfactoryMethodBuilder=proxyTypeBuilder.DefineMethod(MetadataViewGenerator.MetadataViewFactoryName,MethodAttributes.Public|MethodAttributes.Static,typeof(object),CtorArgumentTypes);
ILGeneratorfactoryIL=factoryMethodBuilder.GetILGenerator();
factoryIL.Emit(OpCodes.Ldarg_0);
factoryIL.Emit(OpCodes.Newobj,proxyCtor);
factoryIL.Emit(OpCodes.Ret);

Inner workings

A newobj instruction is logically two operations: (a) allocate a block of zero-inited memory, then (b) call the type's instance ctor over this block of memory. (UninitializedObjectFactory skips this second step.)

// C#objecto=newobject();
// MSILnewobjinstancevoid[System.Private.CoreLib]System.Object::.ctor()
movrcx,0x12345678 ; method table pointer for System.Objectcall <allocator> ; when this returns, rax := zero-inited object instancemovrcx,raxcall System.Object::.ctor() ; essentially, a static method with signature (object @this) -> void

When using Activator.CreateInstance, the runtime uses a generic allocator that can handle many different scenarios for allocating an object whose type is known only at runtime. And while this generic allocator is flexible, there's significant overhead due to all the different checks it needs to perform.

On the other hand, when JITting a newobj instruction, since the JIT knows the destination type statically, it's able to choose from a variety of allocators whose implementations are optimized for various different scenarios. For example, there exists an allocator tailored for small, non-finalizable types. There exists a different allocator tailored for finalizable types. And there exists yet another allocator that's used when instrumentation is enabled.

When an [Uninitialized]ObjectFactory<T> instance is created, it queries the runtime for the address of the allocator that would have been used for an object of type T. All allocators share the signature (MethodTable*) -> O, so once the allocator address is determined we can perform the equivalent of the following IL in order to quickly allocate the object.

ldarg.0// assume arg 0 = MethodTable* of destination typeldarg.1// assume arg1 = allocator function pointercalliobject(nativeint)
ret

If we need to fully construct the object, then we further query the runtime for the address of the code that corresponds to the public parameterless ctor of type T. All public parameterless ctors on reference types share the signature (object) -> void, so once the ctor address is determined we can perform the equivalent of the following IL in order to quickly call the ctor.

ldarg.0// assume arg 0 = return value from UninitializedObjectFactory.CreateUninitializedInstance()ldarg.1// assume arg 1 = public parameterless ctor function pointercallivoid(object)
ret

Putting this all together, we get the following pseudo-codegen for ObjectFactory<T>.CreateInstance.

movrcx, qword ptr [@this + <offset_to_methodtableptr_field>]call qword ptr [@this + <offset_to_allocatorptr_field>]movrcx,raxcall qword ptr [@this + <offset_to_ctorptr_field>]ret

Performance

Below are preliminary numbers for various "create an object" scenarios. Calling new object() directly is the baseline.

MethodMeanRatioScenario
DirectNewobj4.011 ns1.00Calling new object() directly
DynamicMethodNewobj5.527 ns1.31Calling a cached DynamicMethod whose IL stream reads newobj [object]; ret;
ObjectFactoryCreateInstance4.937 ns1.23Calling a cached ObjectFactory<object>'s CreateInstance method

I've also experimented with plumbing this through the existing Activator.CreateInstance pipeline. The perf numbers are not as good as calling ObjectFactory.CreateInstance directly because there's still overhead to querying the static cache and performing any necessary checks, and there's some compat behavior such as TargetInvocationException wrapping that we can't avoid, but they still show a significant improvement over the baseline (master branch) Activator.CreateInstance numbers.

MethodToolchainMeanErrorStdDevRatioRatioSD
ActivatorCreateInstancemaster40.55 ns0.889 ns1.156 ns1.000.00
ActivatorCreateInstanceobjectfactory14.54 ns0.362 ns0.303 ns0.360.01
ActivatorCreateInstanceOfTmaster42.17 ns0.917 ns1.581 ns1.000.00
ActivatorCreateInstanceOfTobjectfactory16.86 ns0.409 ns0.613 ns0.400.02

And for RuntimeHelpers.GetUninitializedObject(typeof(object)) vs. UninitializedObjectFactory<object>.CreateUninitializedInstance():

MethodMeanRatio
RH_GetUninitializedObject40.377 ns1.00
UninitializedObjectFactory3.588 ns0.09

Remaining work

  • Continue with code cleanup and testing edge cases like trying to instantiate arrays.
  • [cut, no new API right now] Support parameterful ctors; e.g., new T(IServiceProvider).
  • Convert the code to use calli support when it's natively available in C#.
  • [cut, no new API right now] Full API review.
  • [cut, no new API right now] Support for Mono + CoreRT.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks

GrabYourPitchforks commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

publicstaticclassObjectFactory{publicstaticFunc<T1,TObject>CreateFactory<TObject,T1>(ConstructorInfoctor);publicstaticFunc<T1,T2,TObject>CreateFactory<TObject,T1,T2>(ConstructorInfoctor);// ...publicstaticFunc<T1,T2,/* ..., */T16,TObject>CreateFactory<TObject,T1,T2,/* ..., */T16>(ConstructorInfoctor);}

I'm not quite sure at the moment how to support this pattern for arbitrary delegate types without involving codegen. In an ideal world I'd be able to expose a generic method as below.

publicstaticclassObjectFactory{publicstaticTDelegateCreateFactory<TDelegate>(ConstructorInfoctor)whereTDelegate:delegate;}privatedelegateICommonInterfaceCreationDelegate(inta,longb,stringc);CreationDelegatedel=ObjectFactory.CreateFactory<CreationDelegate>(someConcreteType.GetConstructor(/* ... */));ICommonInterfacenewlyCreatedObj=del(40,0xdeadbeef,"hello!");

As long we we don't have pointers, refs, or byrefs in the delegate signature I could map this back to a Func<...> and get the appropriate mdtoken for the calli invocation. But as soon as pointers, refs, or byrefs are involved I can't guarantee an appropriate mdtoken can be found, so it could fail unexpectedly.

Edit: Pointers can be smuggled as IntPtr, so that shouldn't be a problem. Refs and byrefs are still a problem. It means we couldn't expose a delegate over the following ctor.

publicclassMyClass:ICommonInterface{publicMyClass(ReadOnlySpan<char>data);}privatedelegateICommonInterfaceFactory(ReadOnlySpan<char>data);Factoryfactory=ObjectFactory.CreateFactory<Factory>(/* some ConstructorInfo */);// fails at runtime

@rynowak

Copy link
Copy Markdown
Contributor

This seems immensely valuable for ASP.NET Core (and I'm sure scores of other libraries), especially if we can get this include more arities, and more kinds of things beyond construction.

One more thing that I want to push onto your stack:

It should also be feasible to write codegen-less stubs for some Func<...>-based factory methods.

We end up needing this kind of thing a lot - BUT in the scenario where we can't write compile-time type-safe code. We end up with Func<object, object[], object> (reciever, args, retval) 😆. It's not really a huge performance problem because we've already written a lot of code to solve it, but it would greatly simplify a lot of our infrastructure if we could have simpler APIs that did the most optimal thing.

If we feel like these problems can only be solved adequately with build-time codegen, then it's possible that the things I'm mentioning here aren't super relevant.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ObjectFactory.cs Outdated
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Potential consumers

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

build-time codegen

Right, build-time codegen is the most optimal thing for this.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Both examples are weakly typed (return object). What is the example for the strongly typed consumer of this?

Possible scenario (though it would require API tweaks):

// calculate and cache these upfrontTypeactualServiceType=environment.GetConcreteTypeForAbstraction(typeof(IMyService));ObjectFactory<IMyService>factory=ObjectFactory.Create<IMyService>(actualServiceType);// call this in a hot path, such as per-requestIMyServiceservice=factory.CreateInstance();

Or, for an MVC-like framework's per-request processing logic:

TypecontrollerType=GetControllerTypeForCurrentRequest();ObjectFactory<Controller>factory=s_factories[controllerType];Controllercontroller=factory.CreateInstance();Console.WriteLine(controller.GetType());// could be AdminController, WeatherController, etc.

There's nothing stopping us from saying "ObjectFactory.CreateInstance always returns object, not T." But IMO there is something nice about an API that returns T instead of object, meaning you don't have to cast the return value and you don't have to worry about putting the wrong category of factory (ObjectFactory<IServiceA> vs. ObjectFactory<IServiceB>) into your static cache, etc.

@marek-safar

Copy link
Copy Markdown
Contributor

If we are designing a new activator-like API we should ensure linking and AOT scenarios needs are addressed.

@jkotas

Copy link
Copy Markdown
Member

Possible scenario (though it would require API tweaks)
ObjectFactory.Create<IMyService>(actualServiceType);

Once you tweak the API like this, you do not need the non-generic version. The non-generic version is equivalent to ObjectFactory<object>.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I'm not actively working on this PR right now. Closing it here so that it doesn't distract the folks who maintain this repo. It's being tracked at GrabYourPitchforks#1 so it doesn't fall entirely off the radar.

@GrabYourPitchforks

GrabYourPitchforks commented May 10, 2020

Copy link
Copy Markdown
MemberAuthor

Rewrote the prototype using the C# native function pointers feature. Here's the codegen for ObjectFactory.CreateInstance routine. It could do with some slightly more efficient register shuffling, but that's just a minor nit.

00007ffd`7410d030 57pushrdi ; prolog00007ffd`7410d031 56pushrsi00007ffd`7410d032 4883ec28 subrsp,28h00007ffd`7410d036 488bf1 movrsi,rcx00007ffd`7410d039 488b4618 movrax,qword ptr [rsi+18h] ; rax := pfnNewHelper00007ffd`7410d03d 488b4e10 movrcx,qword ptr [rsi+10h] ; rcx := pMT00007ffd`7410d041 ffd0 callrax00007ffd`7410d043 488bf8 movrdi,rax00007ffd`7410d046 488b4620 movrax,qword ptr [rsi+20h] ; rax := pfnParameterlessCtor00007ffd`7410d04a 488bcf movrcx,rdi ; rcx := return value from pfnNewHelper00007ffd`7410d04d ffd0 callrax00007ffd`7410d04f 488bc7 movrax,rdi ; rax := fully initialized object instance00007ffd`7410d052 4883c428 addrsp,28h ; epilog00007ffd`7410d056 5e poprsi00007ffd`7410d057 5f poprdi00007ffd`7410d058 c3 ret

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

And here are the perf results from the latest prototype, taking everybody's feedback into account. The first number shows a significant boost to Activator.CreateInstance(Type, bool), caused by replumbing the internal ActivatorCache type through these new code paths.

MethodToolchainMeanErrorStdDevRatioRatioSD
Activator.CreateInstancemaster32.349 ns0.2468 ns0.1927 ns1.000.00
Activator.CreateInstanceproto11.759 ns0.2967 ns0.2775 ns0.360.01

The second number compares the cost of invoking a factory to perform activation. In the first row, the Func<T> has been generated from using a DynamicMethod whose implementation is to newobj the hardcoded target type. In the second row, the factory has been created via Activator.CreateFactory<T>(). These tests don't measure the initial cost of creating the factory; only the cost of invoking them.

ScenarioMeanErrorStdDevRatioRatioSD
From DynamicMethod4.220 ns0.0866 ns0.0811 ns1.000.00
From Activator.CreateFactory(Type, ...)4.689 ns0.0621 ns0.0518 ns1.110.03

This shows that the difference is less than half a nanosecond. Since the factory returned by Activator.CreateFactory(...) doesn't spin up the JIT or involve codegen (beyond what the app itself would have done), it's appropriate for high-performance / fast-startup scenarios.

And just in case people were interested in how long it takes to compile a DynamicMethod vs. call Activator.CreateFactory(...), here are the results. I wouldn't put too much weight on them since this isn't something applications will be doing over and over again - the idea is to cache these. But they're interesting nonetheless.

ScenarioMeanErrorStdDevRatio
Create and compile a DynamicMethod4,602.3 ns87.50 ns166.48 ns1.00
Call Activator.CreateFactory184.1 ns2.31 ns2.16 ns0.04

- Eliminates ObjectFactory<T> except when caller explicitly wants a Func<T>
- Uses C# 9.0 function pointers to avoid introducing new JIT intrinsics
@GrabYourPitchforks

GrabYourPitchforks commented May 11, 2020

Copy link
Copy Markdown
MemberAuthor

I reopened this because it's actively being worked on. Still not ready for review because of build failures, lack of support for C# 9.0 language features, and no mono integration yet. It hasn't gone through more than basic smoke testing yet.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

A status update - I'm still working on this but keep running into issues where the linker either crashes or trims a method which is clearly statically reachable. Trying to work around those before sending up a new iteration. Meanwhile I've reduced the scope of the work so that it's not adding any new APIs in the 5.0 timeframe.

- Remove new public APIs
- Remove most new native APIs
- Plumb GetUninitializedObject atop new native code paths
- Use actual builds from arcade
@GrabYourPitchforks
GrabYourPitchforks marked this pull request as ready for review July 8, 2020 06:04
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

I think all pending PR feedback is addressed, including [largely] restoring the original unmanaged method for GetUninitializedObject, removing the IsNullable helper and related support APIs that were added, and removing all MethodInfo* awareness from the managed side.

The file was also renamed as part of this, so I'm hoping GitHub's diff tool can correctly highlight only the lines that changed.

There was some earlier feedback about debug assertions. I currently have an assertion in each of the APIs ActivatorCache.GetUninitializedObject and ActivatorCache.RunConstructor because an external caller (Activator) is responsible for providing input parameters to these methods, and it seemed prudent to validate that the arguments were valid. If these checks aren't worthwhile I can delete them.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Intermediate commit applied only your suggestions without my fixes to balance out braces, etc. Should be good now.

Comment threadsrc/coreclr/src/vm/reflectioninvocation.cpp Outdated
@jkotas

Copy link
Copy Markdown
Member

👍

@GrabYourPitchforks

GrabYourPitchforks commented Nov 26, 2020

Copy link
Copy Markdown
MemberAuthor

Test failing due to the newly-added asserts: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-32520-merge-1988d13847ae470481/Interop/console.4eecf050.log?sv=2019-07-07&se=2020-12-16T04%3A21%3A35Z&sr=c&sp=rl&sig=TQivJ0aRUNLD3%2FXXQozy90NnIEQZhnk1Nk0aeAoxzqI%3D

 Interop\COM\ComWrappers\GlobalInstance\GlobalInstanceMarshallingTests\GlobalInstanceMarshallingTests.cmd [FAIL]
Process terminated. Assertion failed.
Caller passed an unexpected 'this' parameter to ctor - possible type safety violation.
Expected type: System.__ComObject
Actual type: ComWrappersTests.GlobalInstance.Program+FakeWrapper
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
at System.Activator.CreateInstance(Type type)
at ComWrappersTests.GlobalInstance.Program.ValidateNativeServerActivation() in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 391
at ComWrappersTests.GlobalInstance.Program.ValidateComActivation(Boolean validateUseRegistered) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs:line 380
at ComWrappersTests.GlobalInstance.Program.Main(String[] doNotUse) in /__w/1/s/src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.Marshalling.cs:line 35

Trying to figure out whether the assert indicates a real problem or whether the assert should be removed.

Edit: looks like the runtime behavior is intentional due to the presence of a custom COM marshaler in the sample app.

protectedoverrideobjectCreateObject(IntPtrexternalComObject,CreateObjectFlagsflag)
{
if(ReturnInvalid)
returnnull;
Guid[]iids={
typeof(ITrackerObject).GUID,
typeof(ITest).GUID,
typeof(Server.Contract.IDispatchTesting).GUID,
typeof(Server.Contract.IConsumeNETServer).GUID
};
for(vari=0;i<iids.Length;i++)
{
variid=iids[i];
IntPtrcomObject;
inthr=Marshal.QueryInterface(externalComObject,refiid,outcomObject);
if(hr==0)
returnnewFakeWrapper(comObject);
}
returnnull;
}

I think it's best to remove the assert.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

@jkotas Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr? We're about to turn around and invoke it anyway, so may as well get the fnptr directly to the target method rather than to the prestub?

Perhaps this would make more sense for the Func<object?> scenario, since if somebody is caching an activator delegate then they reasonably expect to invoke it over and over and over again.

@jkotas

Copy link
Copy Markdown
Member

Do you suppose there's any benefit to eagerly JITting the ctor before we obtain its fnptr?

There is still going to be an extra indirection by default because of tiered JIT and other features like profiler rejit.

We pay for these indirections in many other places. The way to fix them is to register the place where the function pointer is stored for backpatching. e.g. the backpatching would have to aware of &_pfnCtor and keep updating as new version of the method gets produced. It is complex to do. We do it only for the hottest indirections like vtable slots.

@jkotas

Copy link
Copy Markdown
Member

Any reasons for still having NO MERGE label? I think this is good to merge.

@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Nah. I'll remove the label and merge in just a sec. Wanted to go over it with Steve early next week once the holiday is finished, but honestly any extra feedback can go in as a follow up PR.

Thank you again for your patience and for sharing your expertise. :)

@GrabYourPitchforksGrabYourPitchforks removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks merged commit ffa24bd into dotnet:masterNov 26, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the fast_createinstance branch November 26, 2020 07:33
@GrabYourPitchforksGrabYourPitchforks changed the title Refactor Activator.CreateInstance and RuntimeHelpers.GetUninitObject to a common code pathImprove performance of Activator.CreateInstanceNov 26, 2020
Comment on lines +51 to +69
catch (Exception ex)
{
// Exception messages coming from the runtime won't include
// the type name. Let's include it here to improve the
// debugging experience for our callers.

string friendlyMessage = SR.Format(SR.Activator_CannotCreateInstance, rt, ex.Message);
switch (ex)
{
case ArgumentException: throw new ArgumentException(friendlyMessage);
case PlatformNotSupportedException: throw new PlatformNotSupportedException(friendlyMessage);
case NotSupportedException: throw new NotSupportedException(friendlyMessage);
case MethodAccessException: throw new MethodAccessException(friendlyMessage);
case MissingMethodException: throw new MissingMethodException(friendlyMessage);
case MemberAccessException: throw new MemberAccessException(friendlyMessage);
}

throw; // can't make a friendlier message, rethrow original exception
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better than, say:

catch(ArgumentExceptione){thrownewArgumentException(CreateFriendlyMessage(rt,e));}catch(PlatformNotSupportedExceptione){thrownewPlatformNotSupportedException(CreateFriendlyMessage(rt,e));}
...// etc.

? I realize the above is probably a bit more IL, but it also means you won't catch and rethrow if the exception didn't match one of the special-cased types. I don't know how common that will be.

@GrabYourPitchforksGrabYourPitchforksDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring this, but leaving this thread marked unresolved for now because I think we can address it as part of #36194.

@adamsitnik

Copy link
Copy Markdown
Member

@GrabYourPitchforks the improvement has been confirmed in DrewScoggins/performance-2#3541

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@GrabYourPitchforks@Dotnet-GitSync-Bot@rynowak@jkotas@marek-safar@SteveSandersonMS@BrzVlad@adamsitnik@hughbe@stephentoub@steveharter@YohDeadfall@MichalStrehovsky