Skip to content
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

Optimization: convert ResolveRequest into readonly struct #1397

Merged
merged 5 commits into from Nov 15, 2023

Conversation

SergeiPavlov
Copy link
Contributor

@SergeiPavlov SergeiPavlov commented Oct 16, 2023

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

  • No need for null-checking
  • Remove GetOrCreateInstanceThrowsArgumentNullExceptionWhenResolveRequestIsNull test as not-applicable anymore
  • Add in function argument specifier to pass pointer instead of struct contents.

@codecov
Copy link

codecov bot commented Oct 16, 2023

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
@@             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 Coverage Δ
src/Autofac/Core/Container.cs 67.74% <ø> (ø)
src/Autofac/Core/ImplicitRegistrationSource.cs 86.84% <ø> (ø)
src/Autofac/Core/Lifetime/LifetimeScope.cs 83.73% <ø> (+0.99%) ⬆️
...Resolving/Pipeline/DefaultResolveRequestContext.cs 87.09% <100.00%> (+3.22%) ⬆️
src/Autofac/Core/Resolving/ResolveOperation.cs 84.09% <ø> (-0.36%) ⬇️
.../Autofac/Diagnostics/DiagnosticSourceExtensions.cs 100.00% <ø> (ø)
...utofac/Diagnostics/OperationStartDiagnosticData.cs 100.00% <100.00%> (ø)
...rc/Autofac/Features/Decorators/DecoratorContext.cs 100.00% <100.00%> (ø)
...eatures/LazyDependencies/LazyRegistrationSource.cs 83.33% <100.00%> (+3.33%) ⬆️
...utofac/Features/Metadata/MetaRegistrationSource.cs 75.00% <ø> (ø)
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alistairjevans
Copy link
Member

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
Copy link
Contributor Author

SergeiPavlov commented Oct 18, 2023

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

Copy link
Member

@alistairjevans alistairjevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issue with the changes here, but adding in is a binary-breaking change, as is changing ResolveRequest to a struct; which really should make this a major version bump.

Sure, most people will use the extension methods, but I almost guarantee someone somewhere has their own extension method that instantiates ResolveRequest and calls ResolveComponent.

We may have to do a release for the .NET 8 release soonish, so I'm tempted to hold this change until we're prepared to do a major version bump.

@tillig
Copy link
Member

tillig commented Nov 6, 2023

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.

@tillig tillig merged commit a518a48 into autofac:develop Nov 15, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants