-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Remove closure allocations from ServiceCollectionDescriptorExtensions #44696
Remove closure allocations from ServiceCollectionDescriptorExtensions #44696
Conversation
Tagging subscribers to this area: @eerhardt, @maryamariyan |
{ | ||
// Already added | ||
return; | ||
} |
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.
How many times are these methods called in a typical startup? The fact that you said it's saving ~750 closures suggests it's called to add that many services, and this is O(N^2) (for each service looking at every other service already added)?
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.
Yep definitely is; can't get the call counts atm as Tracing in dotTrace is broken for me atm 😢
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.
It'd be good to understand the actual cost. While saving 750 allocations is good, I'd expect saving 250,000 interface dispatches to be better ;-)
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.
The service collection needs to store a dictionary and introduce a new interface to check if a service type already exists
Drops the closure allocations for
Func<ServiceDescriptor, Boolean>
from 754 objects to 2 for startup allocations of an ASP.NET MVC appBefore
After
Contributes to #44598