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

[Uri] Uri.ReCreateParts allocates a char[] that could be avoided #22903

Closed
stephentoub opened this issue Jul 24, 2017 · 5 comments · Fixed by #34864
Closed

[Uri] Uri.ReCreateParts allocates a char[] that could be avoided #22903

stephentoub opened this issue Jul 24, 2017 · 5 comments · Fixed by #34864
Assignees
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Milestone

Comments

@stephentoub
Copy link
Member

Accessing a property like Uri.PathAndQuery not only allocates the resulting string, but also a char[] that's often larger than necessary and which is then used to create that resulting string. Whenever/wherever possible, it'd be good to avoid that allocation.

As an example of the impact of this allocation, that one char[] array is currently showing up as ~14% of the memory allocated when making a simple get request with ManagedHandler.

Related to dotnet/corefx#16456
cc: @safern, @geoffkizer, @CIPop

@danmoseley
Copy link
Member

@stephentoub
Copy link
Member Author

stephentoub commented Apr 2, 2018

Here's one way this can be done:
stephentoub/corefx@c28e627

In a little benchmark like:

using System;
using System.Diagnostics;

class Program
{
    static void Main()
    {
        var sw = new Stopwatch();
        var uri = new Uri("http://httpbin.org");
        while (true)
        {
            const int Iters = 20000000;
            long mem = GC.GetAllocatedBytesForCurrentThread();
            sw.Restart();
            for (int i = 0; i < Iters; i++)
            {
                var result = uri.GetComponents(UriComponents.PathAndQuery | UriComponents.Fragment, UriFormat.UriEscaped);
            }
            sw.Stop();
            mem = GC.GetAllocatedBytesForCurrentThread() - mem;
            Console.WriteLine(sw.Elapsed.TotalSeconds + " : " + (mem / (double)Iters));
        }
    }
}

execution time stays approximately the same, but allocation per GetComponents call drops from 336 bytes to 32 bytes.

cc: @rmkerr, @davidsh

@geoffkizer
Copy link
Contributor

geoffkizer commented Apr 2, 2018

More generally, I think there are a bunch of allocations (and probably some CPU) from Uri that we hit in the SocketsHttpHandler path, and that could ideally be avoided. I'm pretty sure we're allocating when we retrieve the Host/IdnHost property, right?

The typical case here (for both Host and PathAndQuery) is that we don't need to escape anything, so being able to access this stuff as ReadOnlySpan<char> seems ideal. We could just allocate and cache in the cases where we do need to escape something.

(Of course ReadOnlySpan<utf8char> would be even better...)

@stephentoub
Copy link
Member Author

Yes, that's just a more significant change.

@geoffkizer
Copy link
Contributor

I know, I'm not suggesting we do anything like that now. I just don't want it to get lost. I'll file a separate issue.

@rmkerr rmkerr changed the title Uri.ReCreateParts allocates a char[] that could be avoided [Uri] Uri.ReCreateParts allocates a char[] that could be avoided May 2, 2018
@stephentoub stephentoub self-assigned this Oct 29, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@stephentoub stephentoub removed the enhancement Product code improvement that does NOT require public API changes/additions label Feb 4, 2020
@stephentoub stephentoub removed their assignment Feb 4, 2020
@stephentoub stephentoub added the enhancement Product code improvement that does NOT require public API changes/additions label Feb 4, 2020
@MihaZupan MihaZupan self-assigned this Feb 21, 2020
@karelz karelz modified the milestones: 5.0.0, Future Jul 28, 2020
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 4, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 22, 2021
@MihaZupan MihaZupan modified the milestones: Future, 6.0.0 Jan 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 21, 2021
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 tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants