-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Regressions in Microsoft.Extensions.DependencyInjection.GetService #54351
Comments
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue DetailsRun Information
Regressions in Microsoft.Extensions.DependencyInjection.GetService
Reprogit clone https:/dotnet/performance.git
python3 .\performance\scripts\benchmarks_ci.py -f netcoreapp5.0 --filter 'Microsoft.Extensions.DependencyInjection.GetService*' PayloadsHistogramMicrosoft.Extensions.DependencyInjection.GetService.ServiceScopeProvider
Microsoft.Extensions.DependencyInjection.GetService.ServiceScope
DocsProfiling workflow for dotnet/runtime repository
|
Looks like it is caused by #53325. |
bug fix > performance 😄 |
Sure, but did we expect a regression from that change? Is the regression caused by the if check? Can we make it cheaper? |
I did yes, it's one extra branch. We should definitely see if we can make the check cheaper but I'm not sure it's worth redesigning anything over. Maybe we can do a reference equals for the root scope check or stash a bool instead of service scope itself to avoid that comparison altogether |
We should try turning IsRootScope into a readonly field. |
- Stash a field instead of doing an equality comparison. Fixes #54351
- Stash a field instead of doing an equality comparison. Fixes #54351
We should leave this open since the change didn't have much of an impact on this code path. |
Looking at the actual benchmark(s) more deeply it seems like:
IServiceScopeFactory is a singleton so I wonder if this part of the change made thing worse. I would have thought that @pakrym's change here would have been a fine replacement for this but maybe avoiding the call to the runtime resolving when the value is cached is faster. I don't get why CreateScope is slower. |
The latest charts show we gained back perf numbers after PR #54732: So this issue can be safely closed. |
Run Information
Regressions in Microsoft.Extensions.DependencyInjection.GetService
Historical Data in Reporting System
Repro
Payloads
Baseline
Compare
Histogram
Microsoft.Extensions.DependencyInjection.GetService.ServiceScopeProvider
Microsoft.Extensions.DependencyInjection.GetService.ServiceScope
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: