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

Add optimisation for reflection-activated components to avoid adding a parameter in the resolve path if possible. #119

Merged
merged 5 commits into from
Apr 27, 2024

Conversation

alistairjevans
Copy link
Member

@alistairjevans alistairjevans commented Apr 13, 2024

The issue this was raised against contains an extremely deep resolve graph, but it does show there's a considerable overhead to changing the request parameters even if a type has no keyed service parameters.

Basically, when components are registering we'll "look ahead" at all the constructors for reflection activation, and if none of them have a FromKeyedService attribute, we can skip adding the additional parameter in the new middleware.

I've also moved the middleware to the start of the activation phase of the component pipeline, because there's no point adding the parameters if we've already instantiated a shared component.

Point of Debate

@tillig, one question for you is in the review comment, about whether we need to support a certain esoteric use-case; if not then we get to stay fast in the delegate component path.

Closes #118

Benchmarks

This is the same slightly extreme benchmark used to raise #118

v8

|            Method |     Mean |   Error |  StdDev |    Gen 0 |   Gen 1 | Allocated |
|------------------ |---------:|--------:|--------:|---------:|--------:|----------:|
| DirectResolveDeep | 677.2 us | 5.15 us | 4.56 us | 105.4688 | 34.1797 |    462 KB |

v9

| Method            | Mean     | Error     | StdDev    | Gen0     | Gen1    | Allocated |
|------------------ |---------:|----------:|----------:|---------:|--------:|----------:|
| DirectResolveDeep | 1.637 ms | 0.0378 ms | 0.1092 ms | 160.1563 | 56.6406 | 739.33 KB |

This branch

| Method            | Mean     | Error    | StdDev   | Gen0     | Gen1    | Allocated |
|------------------ |---------:|---------:|---------:|---------:|--------:|----------:|
| DirectResolveDeep | 670.4 us | 10.49 us | 10.77 us | 104.4922 | 27.3438 | 461.86 KB |

Copy link

codecov bot commented Apr 13, 2024

Codecov Report

Attention: Patch coverage is 98.14815% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.69%. Comparing base (aa5edfe) to head (a690c3c).

Files Patch % Lines
...ions.DependencyInjection/KeyedServiceMiddleware.cs 95.83% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #119      +/-   ##
===========================================
+ Coverage    88.70%   90.69%   +1.98%     
===========================================
  Files           16       15       -1     
  Lines          310      344      +34     
  Branches        57       69      +12     
===========================================
+ Hits           275      312      +37     
+ Misses          22       20       -2     
+ Partials        13       12       -1     

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

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

I think we can drop the delegate support to make this a touch faster; and if the unit test can be added to satisfy coverage, I think we're good to go.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

🐲

@tillig tillig merged commit dc03f10 into develop Apr 27, 2024
4 checks passed
@tillig tillig deleted the bug/keyed-middleware-performance branch April 27, 2024 14:09
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.

Performance Degradation in version 9.0.0
2 participants