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

Improve Uri.UnescapeDataString codepath -> UriHelper.UnescapeString #20177

Closed
safern opened this issue Feb 10, 2017 · 6 comments · Fixed by dotnet/corefx#41684
Closed

Improve Uri.UnescapeDataString codepath -> UriHelper.UnescapeString #20177

safern opened this issue Feb 10, 2017 · 6 comments · Fixed by dotnet/corefx#41684
Assignees
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Milestone

Comments

@safern
Copy link
Member

safern commented Feb 10, 2017

After doing an investigation on some performance traces collected from some mvc aspnet applications scenarios we found that a considerable amount of char[] objects were allocated on the GC Heap which are coming from Uri.UnescapeDataString codepath that is calling UriHelper.UnescapeString (source code) which receives a char[] as the destination of the unescaped string and then creating a string from that char[].

I know it would be a big change but we could probably consider using the sourceString that is already allocated or using a PooledStringBuilder which uses an ArrayPool for the buffer cache as explained here:

/cc @danmosemsft @jkotas @stephentoub

@danmoseley
Copy link
Member

Concretely in the MVC scenario captured, UnescapeDataString is 1% of CPU time and 1.5% of strings (which doesn't sound like much but there's a lot of strings in this scenario). There really doesn't seem like a great reason to make a char[] and certainly not to make a new string out of it given probably most times the result will be the same as the input.

@safern safern self-assigned this Feb 15, 2017
@safern
Copy link
Member Author

safern commented Feb 15, 2017

Currently working on this

@karelz
Copy link
Member

karelz commented Mar 4, 2017

Flipping back to "up for grabs"
Next steps: Pick up dotnet/corefx#16456 and finish it.
Complexity: Medium

@safern
Copy link
Member Author

safern commented Mar 4, 2017

Just FYI: I had an implementation proposal but as we had other priorities now I stopped pursuing that but I did see improvements. Here is the PR whit my proposal: dotnet/corefx#16456

@stephentoub
Copy link
Member

See https://github.com/dotnet/corefx/issues/22569#issuecomment-377836840 and stephentoub/corefx@c28e627, which also covers these code paths.

@alnikola alnikola self-assigned this Oct 8, 2019
@jkotalik
Copy link
Contributor

jkotalik commented Oct 8, 2019

In Kestrel, we have a class which does path normalization, which has similar concerns to Unescaping. May give some inspiration for ways to improve the perf of UnescapeDataString: https://github.com/aspnet/AspNetCore/blob/master/src/Servers/Kestrel/Core/src/Internal/Http/PathNormalizer.cs

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
None yet
7 participants