-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Improve perf of ActivatorUtilities.CreateInstance() #91290
Conversation
Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection Issue DetailsFixes #91186 We should consider porting to v8 since this addresses a ~3x perf regression in certain cases. This gets back the v8 regressions in ActivatorUtilities.CreateInstance plus a lot more. The regressions were caused by the new v8 "keyed services" feature and are most pronounced when a service's constructor contains many other services. The optimizations include caching of
Benchmarks from issue #91186: ~2.1-4.5x faster than 8.0 and ~1.3x-2.4x faster than 7.0 and with fewer allocs7.0
8.0
This PR
Existing benchmarks: CreateInstance_5 is ~3.5x faster than 8.0 and ~3.3 faster than 7.0 and with half the allocs7.0
8.0
This PR
|
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated
Show resolved
Hide resolved
…onstructorInvoker for possible risk of callstack usage
...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you!
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6146835734 |
Fixes #91186
We should consider porting to v8 since this addresses a ~3x perf regression (or more) in certain cases.
This gets back the v8 regressions in ActivatorUtilities.CreateInstance plus a lot more. The regressions were caused by the new v8 "keyed services" feature and are most pronounced when a service's constructor contains many other services.
The optimizations include caching of
ConstructorInfo
-related state, in order of perf gain:[FromKeyedServicesAttribute]
attribute lookup (the v8 regression)Type.GetConstructors()
along with eachConstructorInfo.Parameters
[ActivatorUtilitiesConstructorAttribute]
attribute lookup.Benchmarks from issue #91186: ~2.1-4.5x faster than 8.0 and ~1.3x-2.4x faster than 7.0 and with fewer allocs
7.08.0
This PR
Existing benchmarks: CreateInstance_5 is ~3.5x faster than 8.0 and ~3.3 faster than 7.0 and with half the allocs
7.0
8.0
This PR