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

Use new Dictionary AlternateLookup to avoid allocation in DictionaryJumpTable #58305

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andrewjsaid
Copy link
Contributor

Use new Dictionary AlternateLookup to avoid allocation in DictionaryJumpTable

Description

Simple change which removes the substring allocation in DictionaryJumpTable using the GetAlternateLookup<ReadOnlySpan<char>> feature of .NET 9.

With this change all JumpTable implementations can accept ReadOnlySpan<char> as a parameter instead of string, PathSegment. I'd be happy to include that change if it's wanted.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Oct 8, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 8, 2024
Copy link
Contributor

Thanks for your PR, @andrewjsaid. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@BrennanConroy
Copy link
Member

Awesome, was looking forward to aspnetcore adopting the new alternate lookup feature.

Could you run the micro-benchmarks before and after this change?
https://github.com/dotnet/aspnetcore/blob/main/src/Http/Routing/perf/Microbenchmarks/Matching/JumpTableMultipleEntryBenchmark.cs

@andrewjsaid
Copy link
Contributor Author

andrewjsaid commented Oct 8, 2024

Interestingly enough the PR looks to be slightly slower (5ns -> 11ns) with the benchmark as-is whereas I would have expected no difference.

However there's a flaw with the benchmark in that it currently always compares the entire string which is unusual and which negates the benefit proposed in this PR. In most real-world cases the PathSegment is only part of the string and so substrings are allocated.

I've modified the benchmark to account for this by using only part of the path string. The modified benchmark results are below. Overall less significant than I would have liked but it's faster and allocates less memory but only if we accept PathSegment usually points to a substring of Path. [edit: I confirmed that this is always the case since all paths start with /]

Before:

Method Count Mean Error StdDev Op/s Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
Baseline 2 0.6304 ns 0.0052 ns 0.0049 ns 1,586,354,557.5 1.00 0.00 - - - -
Dictionary 2 13.9512 ns 0.1030 ns 0.0860 ns 71,678,284.5 22.13 0.29 0.0011 - - 82 B
Baseline 5 0.6195 ns 0.0067 ns 0.0060 ns 1,614,331,173.5 1.00 0.00 - - - -
Dictionary 5 14.5809 ns 0.1890 ns 0.1578 ns 68,582,745.6 23.55 0.32 0.0011 - - 82 B
Baseline 10 0.6169 ns 0.0111 ns 0.0104 ns 1,620,884,005.6 1.00 0.00 - - - -
Dictionary 10 15.2295 ns 0.2681 ns 0.3486 ns 65,661,888.5 24.69 0.86 0.0011 - - 82 B
Baseline 25 0.6165 ns 0.0060 ns 0.0056 ns 1,621,971,184.8 1.00 0.00 - - - -
Dictionary 25 16.9301 ns 0.2896 ns 0.2568 ns 59,066,511.0 27.47 0.46 0.0011 - - 82 B
Baseline 50 0.6172 ns 0.0060 ns 0.0056 ns 1,620,306,284.5 1.00 0.00 - - - -
Dictionary 50 18.1510 ns 0.0577 ns 0.0482 ns 55,093,297.9 29.42 0.34 0.0011 - - 82 B
Baseline 100 0.6176 ns 0.0070 ns 0.0066 ns 1,619,194,748.0 1.00 0.00 - - - -
Dictionary 100 20.4224 ns 0.1325 ns 0.1174 ns 48,965,762.2 33.04 0.38 0.0011 - - 82 B

After:

Method Count Mean Error StdDev Op/s Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
Baseline 2 0.6283 ns 0.0088 ns 0.0082 ns 1,591,566,799.9 1.00 0.00 - - - -
Dictionary 2 11.8230 ns 0.1413 ns 0.1321 ns 84,580,838.3 18.82 0.22 - - - -
Baseline 5 0.6149 ns 0.0052 ns 0.0046 ns 1,626,151,413.8 1.00 0.00 - - - -
Dictionary 5 11.8060 ns 0.1567 ns 0.1466 ns 84,702,576.7 19.19 0.33 - - - -
Baseline 10 0.6141 ns 0.0067 ns 0.0063 ns 1,628,406,229.3 1.00 0.00 - - - -
Dictionary 10 12.0957 ns 0.1984 ns 0.1856 ns 82,673,853.8 19.70 0.39 - - - -
Baseline 25 0.6163 ns 0.0044 ns 0.0039 ns 1,622,602,556.1 1.00 0.00 - - - -
Dictionary 25 13.0903 ns 0.2482 ns 0.2200 ns 76,392,361.3 21.24 0.34 - - - -
Baseline 50 0.6159 ns 0.0095 ns 0.0089 ns 1,623,584,223.2 1.00 0.00 - - - -
Dictionary 50 14.6091 ns 0.0292 ns 0.0273 ns 68,450,281.6 23.72 0.35 - - - -
Baseline 100 0.6143 ns 0.0078 ns 0.0073 ns 1,627,848,658.0 1.00 0.00 - - - -
Dictionary 100 16.1624 ns 0.0569 ns 0.0475 ns 61,871,831.2 26.28 0.34 - - - -

@@ -27,7 +27,7 @@ public void Setup()

for (var i = 0; i < _strings.Length; i++)
{
_segments[i] = new PathSegment(0, _strings[i].Length);
_segments[i] = new PathSegment(1, _strings[i].Length - 2);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be - 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I attempted to trim one from the start and one from the end but I don't suppose it matters too much as long as it's a substring.

@andrewjsaid
Copy link
Contributor Author

I've created dotnet/runtime#108732 which if accepted/merged should further improve the performance of the AlternateLookup used in this PR.

Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants