Skip to content

Reduce Uri ctor overhead#36915

Merged
MihaZupan merged 4 commits intodotnet:masterfrom
MihaZupan:uri-temp2
May 31, 2020
Merged

Reduce Uri ctor overhead#36915
MihaZupan merged 4 commits intodotnet:masterfrom
MihaZupan:uri-temp2

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented May 23, 2020 •
edited
Loading

Reduces the initial overhead for every Uri ctor.

public class UriInitOverhead
{
    [Benchmark]
    public Uri Ascii() => new Uri("http://foo/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");

    [Benchmark]
    public Uri PercentEncoded() => new Uri("http://foo/%01%02%03%04%0F%2F%3F%7F%01%02%03%04%0F%2F%3F%7F");
}
Method Toolchain Mean Error StdDev Ratio
Ascii \perf-base\CoreRun.exe 226.3 ns 0.42 ns 0.33 ns 1.42
Ascii \perf-new\CoreRun.exe 159.6 ns 1.63 ns 1.53 ns 1.00
PercentEncoded \perf-base\CoreRun.exe 499.3 ns 1.13 ns 1.00 ns 2.62
PercentEncoded \perf-new\CoreRun.exe 190.3 ns 0.52 ns 0.44 ns 1.00

Will also improve perf for any processing that calls into UriHelper.EscapedAscii - anywhere we unescape percent-encoded values.

@ghost
Copy link

ghost commented May 23, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@MihaZupan MihaZupan changed the title Uri temp2 Reduce Uri ctor overhead May 23, 2020
@MihaZupan MihaZupan added this to the 5.0 milestone May 23, 2020
@MihaZupan MihaZupan requested a review from a team May 23, 2020 02:15
@gfoidl
Copy link
Member

gfoidl commented May 23, 2020

Should this benchmarks be incorporated to Perf_Uri?

@MihaZupan
Copy link
Member Author

MihaZupan commented May 23, 2020

Yes, the list of Uri-related benchmarks should be extended in the perf repo

danmoseley
danmoseley reviewed May 23, 2020
View reviewed changes
@jaredpar jaredpar mentioned this pull request May 25, 2020
Closed
@MihaZupan
Copy link
Member Author

MihaZupan commented May 26, 2020

The change in the hex decoding also improves perf for Uri.UnescapeDataString

[Benchmark]
public string UnescapeDataString_ReservedAscii() => Uri.UnescapeDataString("%3F%20%3E%21%3F%20%3E%21%3F%20%3E%21%3F%20%3E%21%3F%20%3E%21");
Toolchain Mean Error StdDev Median Ratio RatioSD
\perf-base\CoreRun.exe 230.9 ns 2.51 ns 3.60 ns 229.3 ns 1.20 0.02
\perf-new\CoreRun.exe 192.5 ns 3.62 ns 5.30 ns 196.7 ns 1.00 0.00

stephentoub
stephentoub reviewed May 26, 2020
View reviewed changes
src/libraries/System.Private.Uri/src/System/UriExt.cs
if (c == '%')
{
if (i + 2 < data.Length)
if ((uint)(i + 2) < (uint)data.Length)
Copy link
Member

@stephentoub stephentoub May 26, 2020

Choose a reason for hiding this comment

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

Same question as on the other PR; does this uint casting actually enable the JIT to eliminate the i+1 and i+2 bounds checks? If it does, great; I just didn't think it did.

Copy link
Member Author

@MihaZupan MihaZupan May 31, 2020

Choose a reason for hiding this comment

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

I measured it against a completely unsafe version and the perf is: (new being the unsafe variant)

Method Toolchain Mean Error StdDev Median Ratio RatioSD
Ascii \perf-base\CoreRun.exe 157.4 ns 0.26 ns 1.33 ns 157.0 ns 0.95 0.01
Ascii \perf-new\CoreRun.exe 165.7 ns 0.34 ns 1.68 ns 165.5 ns 1.00 0.00
PercentEncoded \perf-base\CoreRun.exe 192.7 ns 1.94 ns 9.67 ns 186.9 ns 0.98 0.05
PercentEncoded \perf-new\CoreRun.exe 196.7 ns 0.25 ns 1.22 ns 196.6 ns 1.00 0.00

That said, I'll merge it as-is

@MihaZupan MihaZupan merged commit d71ae5f into dotnet:master May 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danmoseley danmoseley danmoseley left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees

No one assigned

Labels

area-System.Net

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan @gfoidl @stephentoub @danmoseley @Dotnet-GitSync-Bot