Skip to content

Avoid allocating the CandidateSet when there's a single match #9622

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

Merged
merged 3 commits into from
Apr 22, 2019

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Apr 21, 2019

  • Add fast path for 0 route values, and policies

Scenario, hello world plaintext (the /plaintext sample in the RoutingSandbox) hit with 10K requests.

Allocations before:

image

Allocations after:

image

Microbenchmarks:

MatcherSingleEntryBenchmark

Before:

           Method |     Mean |     Error |    StdDev |        Op/s | Scaled | ScaledSD |  Gen 0 | Allocated |
----------------- |---------:|----------:|----------:|------------:|-------:|---------:|-------:|----------:|
         Baseline | 106.5 ns |  1.762 ns |  1.472 ns | 9,387,641.5 |   1.00 |     0.00 | 0.0074 |      40 B |
              Dfa | 185.6 ns |  3.724 ns |  8.556 ns | 5,388,922.3 |   1.74 |     0.08 | 0.0212 |     112 B |
 LegacyTreeRouter | 954.0 ns | 20.355 ns | 38.231 ns | 1,048,254.6 |   8.96 |     0.37 | 0.0458 |     408 B |
     LegacyRouter | 711.2 ns | 13.520 ns | 11.985 ns | 1,406,107.4 |   6.68 |     0.14 | 0.0525 |     280 B |

After:

           Method |      Mean |      Error |     StdDev |         Op/s | Scaled | ScaledSD |  Gen 0 | Allocated |
----------------- |----------:|-----------:|-----------:|-------------:|-------:|---------:|-------:|----------:|
         Baseline |  96.00 ns |  0.8115 ns |  0.7193 ns | 10,416,201.9 |   1.00 |     0.00 | 0.0076 |      40 B |
              Dfa |  97.07 ns |  1.0586 ns |  0.9902 ns | 10,301,416.6 |   1.01 |     0.01 |      - |       0 B |
 LegacyTreeRouter | 913.16 ns | 17.9336 ns | 17.6132 ns |  1,095,100.5 |   9.51 |     0.19 | 0.0772 |     408 B |
     LegacyRouter | 679.49 ns | 10.7855 ns | 10.0888 ns |  1,471,691.7 |   7.08 |     0.11 | 0.0534 |     280 B |

- Add fast path for 0 route values, and policies
@@ -24,7 +24,7 @@ public sealed class EndpointSelectorContext : IEndpointFeature, IRouteValuesFeat
/// </summary>
public RouteValueDictionary RouteValues
{
get => _routeValues;
get => _routeValues ?? (_routeValues = new RouteValueDictionary());
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to do this, we can take advantage of it in more cases, including the ones that involve cnadidate set.

Copy link
Member

@JamesNK JamesNK Apr 21, 2019

Choose a reason for hiding this comment

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

This is beyond the scope of this PR, but what about reusing RVD instances? It would be similar to how we're reusing HttpContext et al. It's basically a per-request allocation that is tied to the route values feature.

Reusing RVD, and the routing feature, would save allocations on every request, not just those with no route values.

Caching per-request objects on reused HttpContexts

Copy link
Member Author

Choose a reason for hiding this comment

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

We can just straight up do this in Kestrel I fear we won't be able to do the more generic caching for 3.0

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're going to do this, we can take advantage of it in more cases, including the ones that involve cnadidate set.

Yes, we can, I just wanted to minimize the code change for now. I have to look at see what things are assuming route values are never null when looking at CandidateState.RouteValues (maybe I can do it statiaclly with #enable nullable 😄 )

Copy link
Member

Choose a reason for hiding this comment

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

CandidateSet has some special semantics around RVD that should just be removed. If treats null RVD as a shorthand for not-a-match in some APIs.

@rynowak
Copy link
Member

rynowak commented Apr 21, 2019

####?s

@davidfowl
Copy link
Member Author

####?s

Will get some. I went awaty but now I'm back


public DfaMatcher(ILogger<DfaMatcher> logger, EndpointSelector selector, DfaState[] states, int maxSegmentCount)
{
_logger = logger;
_selector = selector;
_states = states;
_maxSegmentCount = maxSegmentCount;
_isDefaultEndpointSelector = selector is DefaultEndpointSelector;
Copy link
Member

Choose a reason for hiding this comment

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

I'd feel slightly better if this was an exact type equality check. This isn't public, but that would capture the intent better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn’t it sealed? Future proofing?

@Tratcher Tratcher removed their request for review April 22, 2019 04:56
@davidfowl davidfowl merged commit d62d33c into master Apr 22, 2019
@davidfowl davidfowl deleted the davidfowl/less-alloc branch April 22, 2019 06:34
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.

3 participants