-
-
Notifications
You must be signed in to change notification settings - Fork 835
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Optimization: convert ResolveRequest
into readonly struct
#1397
Conversation
Codecov ReportAttention:
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
☔ View full report in Codecov by Sentry. |
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 If you're unsure on how to run our benchmarks (under the 'bench' folder), let me know. |
Sure. Here are the results: Branch develop: ChildScopeResolve
DeepGraphResolve
============== Branch struct_ResolveRequest (THIS) ChildScopeResolve
DeepGraphResolve
|
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 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.
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. |
This will save GC work (one allocation per resolving).
Also:
GetOrCreateInstanceThrowsArgumentNullExceptionWhenResolveRequestIsNull
test as not-applicable anymorein
function argument specifier to pass pointer instead of struct contents.