autofac / Autofac

An addictive .NET IoC container
https://autofac.org
MIT License
4.44k stars 836 forks source link

Optimization: convert `ResolveRequest` into readonly struct #1397

Closed SergeiPavlov closed 8 months ago

SergeiPavlov commented 9 months ago

This will save GC work (one allocation per resolving). Also:

codecov[bot] commented 9 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5af8326) 78.47% compared to head (095a0c3) 78.49%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1397 +/- ## =========================================== + Coverage 78.47% 78.49% +0.02% =========================================== Files 201 201 Lines 5715 5716 +1 Branches 1169 1169 =========================================== + Hits 4485 4487 +2 Misses 716 716 + Partials 514 513 -1 ``` | [Files](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac) | Coverage Δ | | |---|---|---| | [src/Autofac/Core/Container.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvQ29yZS9Db250YWluZXIuY3M=) | `67.74% <ø> (ø)` | | | [src/Autofac/Core/ImplicitRegistrationSource.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvQ29yZS9JbXBsaWNpdFJlZ2lzdHJhdGlvblNvdXJjZS5jcw==) | `86.84% <ø> (ø)` | | | [src/Autofac/Core/Lifetime/LifetimeScope.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvQ29yZS9MaWZldGltZS9MaWZldGltZVNjb3BlLmNz) | `83.73% <ø> (+0.99%)` | :arrow_up: | | [...Resolving/Pipeline/DefaultResolveRequestContext.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvQ29yZS9SZXNvbHZpbmcvUGlwZWxpbmUvRGVmYXVsdFJlc29sdmVSZXF1ZXN0Q29udGV4dC5jcw==) | `87.09% <100.00%> (+3.22%)` | :arrow_up: | | [src/Autofac/Core/Resolving/ResolveOperation.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvQ29yZS9SZXNvbHZpbmcvUmVzb2x2ZU9wZXJhdGlvbi5jcw==) | `84.09% <ø> (-0.36%)` | :arrow_down: | | [.../Autofac/Diagnostics/DiagnosticSourceExtensions.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvRGlhZ25vc3RpY3MvRGlhZ25vc3RpY1NvdXJjZUV4dGVuc2lvbnMuY3M=) | `100.00% <ø> (ø)` | | | [...utofac/Diagnostics/OperationStartDiagnosticData.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvRGlhZ25vc3RpY3MvT3BlcmF0aW9uU3RhcnREaWFnbm9zdGljRGF0YS5jcw==) | `100.00% <100.00%> (ø)` | | | [...rc/Autofac/Features/Decorators/DecoratorContext.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvRmVhdHVyZXMvRGVjb3JhdG9ycy9EZWNvcmF0b3JDb250ZXh0LmNz) | `100.00% <100.00%> (ø)` | | | [...eatures/LazyDependencies/LazyRegistrationSource.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvRmVhdHVyZXMvTGF6eURlcGVuZGVuY2llcy9MYXp5UmVnaXN0cmF0aW9uU291cmNlLmNz) | `83.33% <100.00%> (+3.33%)` | :arrow_up: | | [...utofac/Features/Metadata/MetaRegistrationSource.cs](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac#diff-c3JjL0F1dG9mYWMvRmVhdHVyZXMvTWV0YWRhdGEvTWV0YVJlZ2lzdHJhdGlvblNvdXJjZS5jcw==) | `75.00% <ø> (ø)` | | | ... and [2 more](https://app.codecov.io/gh/autofac/Autofac/pull/1397?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=autofac) | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alistairjevans commented 9 months ago

Hi @SergeiPavlov, could you please run a few of our benchmarks (I'm thinking DeepGraphResolve and ChildScopeResolve at least) and show before and after numbers here to prove that the benefit of allocation removal outweighs the one or two places we copy ResolveRequest into a class field in terms of the size of those classes and the copy time?

If you're unsure on how to run our benchmarks (under the 'bench' folder), let me know.

SergeiPavlov commented 9 months ago

Sure. Here are the results:

Branch develop:

ChildScopeResolve

Method Mean Error StdDev Gen0 Gen1 Allocated
Resolve 26.78 us 0.277 us 0.272 us 3.7231 1.8311 30.51 KB
ResolveNeverRegisteredFromChild 13.15 us 0.067 us 0.053 us 2.0905 1.0376 17.1 KB

DeepGraphResolve

Method Mean Error StdDev Gen0 Gen1 Allocated
Resolve 9.775 us 0.0253 us 0.0249 us 1.7700 0.0458 14.54 KB

==============

Branch struct_ResolveRequest (THIS)

ChildScopeResolve

Method Mean Error StdDev Gen0 Gen1 Allocated
Resolve 26.97 us 0.270 us 0.341 us 3.6621 1.8005 30.13 KB
ResolveNeverRegisteredFromChild 12.87 us 0.033 us 0.029 us 2.0905 1.0376 17.12 KB

DeepGraphResolve

Method Mean Error StdDev Gen0 Gen1 Allocated
Resolve 9.438 us 0.1794 us 0.2395 us 1.6937 0.0305 13.85 KB
tillig commented 8 months ago

Related: autofac/Autofac.Extensions.DependencyInjection#112

I'm not sure we can support M.E.DI keyed services without core Autofac changes. If we have to cut a new/breaking version for .NET 8, it might be worth trying to get these keyed service updates in place at the same time or we could end up with two breaking changes in a row.