-
Notifications
You must be signed in to change notification settings - Fork 751
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
Split scope resolution logic into seperate dynamic methods #570
Conversation
Some perf numbers: Before:
After:
Simple case didn't regress while large tree case improved 4x just because of reduced method size. |
src/DependencyInjection/DI/src/DependencyInjectionEventSource.cs
Outdated
Show resolved
Hide resolved
src/DependencyInjection/DI/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs
Outdated
Show resolved
Hide resolved
var info = ILEmitCallSiteAnalyzer.Instance.CollectGenerationInfo(callSite); | ||
var runtimeContext = GenerateMethodBody(callSite, dynamicMethod.GetILGenerator(info.Size), info); | ||
|
||
#if SAVE_ASSEMBLY |
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.
Do we have a test for this? Apparently this should have been SAVE_ASSEMBLIES even before this change.
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.
This is a feature I only use when debugging code IL code generation it's always excluded from shipping builds.
src/DependencyInjection/DI/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs
Outdated
Show resolved
Hide resolved
src/DependencyInjection/DI/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs
Show resolved
Hide resolved
src/DependencyInjection/DI/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs
Outdated
Show resolved
Hide resolved
return scope => _runtimeResolver.Resolve(callSite, scope); | ||
private GeneratedMethod BuildType(ServiceCallSite callSite) | ||
{ | ||
// Only scope methods are cached |
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.
I'm probably missing something obvious, but why not cache GeneratedMethods for transient services?
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.
I think I remember now. It's because the methods that don't need to check the ResolvedServices dictionary are already small enough. Still, if there's no perf impact of splitting scoped service instantiation into individual methods, it might be worth trying for non-scoped services.
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.
My current thinking is that there is no perf impact because scope resolution methods themselves are comparably slow. There is also not much benefit in caching transient resolvers because most of them contain a single method call anyway.
Goal is to decrease method size in cases of large service trees with a lot of scoped services.
This change is based on #546.
I'll rebase once it goes in.
Fixes dotnet/aspnetcore#3054 and dotnet/aspnetcore#2737
Perf doesn't seem to be impacted.