Skip to content

Removed bound checks in SharedUrlHelper.IsLocalUrl and use JIT unrolling for checks #61361

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

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

Conversation

gfoidl
Copy link
Member

@gfoidl gfoidl commented Apr 7, 2025

The method is used for HTTP Redirects to check if the url is local or not.

The changes to the code are to help the JIT to elide the bound checks, and to collapse some checks (e.g. for '~' and '/') to only one check.

Benchmarking gives on my machine:

Method Url Mean Error StdDev Ratio RatioSD
Default / 1.080 ns 0.0682 ns 0.1377 ns 1.02 0.19
PR / 1.159 ns 0.0334 ns 0.0279 ns 1.09 0.15
Default /foo 3.688 ns 0.1248 ns 0.3662 ns 1.01 0.14
PR /foo 1.976 ns 0.0816 ns 0.1875 ns 0.54 0.07
Default ~/bar 4.307 ns 0.1295 ns 0.1330 ns 1.00 0.04
PR ~/bar 1.687 ns 0.0532 ns 0.0444 ns 0.39 0.02

@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 Apr 7, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 7, 2025
Copy link
Contributor

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

@martincostello
Copy link
Member

What's the impact on allocations if you add [MemoryDiagnoser] to your benchmark?

@gfoidl
Copy link
Member Author

gfoidl commented Apr 7, 2025

There are either way no allocations at all.

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 Apr 14, 2025
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.

2 participants