Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory #89573

Merged
merged 6 commits into from
Aug 3, 2023

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Jul 27, 2023

Fixes #66153 which also contains additional information on benchmarks here.

This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).

In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.

Background:

Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.

@ghost
Copy link

ghost commented Jul 27, 2023

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

Issue Details

[verifying tests]

Author: steveharter
Assignees: steveharter
Labels:

tenet-performance, area-Extensions-DependencyInjection

Milestone: -

@steveharter steveharter added this to the 8.0.0 milestone Jul 27, 2023
@steveharter steveharter requested a review from buyaa-n July 27, 2023 16:58
@steveharter steveharter marked this pull request as ready for review July 27, 2023 17:09
@@ -179,7 +180,7 @@ public void CreateInstance_ClassWithABC_MultipleCtorsWithSameLength_ThrowsAmbigu
}

[Fact]
public void CreateFactory_CreatesFactoryMethod()
public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
Copy link
Member Author

Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?

if (serviceProvider is null)
ThrowHelperArgumentNullExceptionServiceProvider();

object?[] constructorArguments = new object?[parameters.Length];
Copy link
Member

Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.

@steveharter
Copy link
Member Author

Do we have any perf numbers (both throughput and size) for this change?

See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.

See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.

Comment on lines +731 to +732
if (serviceProvider is null)
ThrowHelperArgumentNullExceptionServiceProvider();
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!

@steveharter steveharter merged commit c416966 into dotnet:main Aug 3, 2023
103 checks passed
@steveharter steveharter deleted the DiPerf branch August 3, 2023 12:31
@jkotas
Copy link
Member

jkotas commented Aug 4, 2023

This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.

Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result

[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
   at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
   at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
   at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
   at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
   at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
   at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
   at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit that referenced this pull request Aug 4, 2023
AaronRobinsonMSFT pushed a commit that referenced this pull request Aug 4, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible
6 participants