Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Reduce allocations in CookieContainer.GetCookieHeader#7285

Merged
stephentoub merged 2 commits intodotnet:masterfrom
stephentoub:cookie_allocs
Mar 29, 2016
Merged

Reduce allocations in CookieContainer.GetCookieHeader#7285
stephentoub merged 2 commits intodotnet:masterfrom
stephentoub:cookie_allocs

Conversation

@stephentoub
Copy link
Member

HttpClient defaults to UseCookies=true, which means by default every request ends up asking the CookieContainer to get the cookie header that should be used. Today, even if there aren't any cookies for the given Uri, this ends up doing a lot of allocation. And if there are cookies for the Uri, the serialization of each cookie results in a handful of strings, char[]s, and string[] allocations. This commit tweaks the implementation to avoid a bunch of these, including taking advantage of StringBuilderCache. (There are still some more that could be cleaned up with further work, but it becomes much more invasive.)

For this sample program:

CookieContainer cc = new CookieContainer();

Uri exists = new Uri("http://example.com/exists");
Uri notExists = new Uri("http://example.com/notExists");

cc.Add(exists, new Cookie("test1", "value1"));
cc.Add(exists, new Cookie("test2", "value2"));

for (int i = 0; i < 1000; i++)
{
    cc.GetCookieHeader(exists);
}

before these changes, I see the following allocations:
image
and after thes changes:
image

If I then change the GetCookieHeader line to be:

cc.GetCookieHeader(notExists);

before these changes, I see the following allocations:
image
and after these changes:
image

cc: @davidsh, @CIPop, @ericeil

HttpClient defaults to UseCookies=true, which means by default every request ends up asking the CookieContainer to get the cookie header that should be used. Today, even if there aren't any cookies for the Uri, this ends up allocating a CookieCollection, a StringBuilder, a couple of List<T>s, an iterator, etc., none of which are actually necessary.  And if there are cookies for the Uri, the serialization of each cookie results in a handful of strings, char[]s, and string[] allocations. This commit fixes the implementation to avoid these. (There are still some that could be cleaned up with further work, but it becomes much more invasive.)
@@ -2,10 +2,11 @@
// The .NET Foundation licenses this file to you under the MIT license.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should consider making equivalent changes to the copy of Cookie.cs here:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/netcore50/System/Net/cookie.cs

Long term....there won't be a copy of the file. But due to the way that netcore50 needs cookie functionality, it needed to duplicate the file for now....could potentially be refactored to common code, etc.

@stephentoub
Copy link
Member Author

Thanks, @davidsh. I ported the Cookie changes over to cookie.cs. I decided not to use StringBuilderCache (yet), since it's not currently used in the Http library, but that's something we likely want to do for other reasons soon.

@davidsh
Copy link
Contributor

davidsh commented Mar 29, 2016

LGTM

@stephentoub
Copy link
Member Author

Thanks, @davidsh. Do you want to run any .NET Native tests before this is merged?

@davidsh
Copy link
Contributor

davidsh commented Mar 29, 2016

I'm ok w/ merging now.

@stephentoub stephentoub merged commit f22362d into dotnet:master Mar 29, 2016
@stephentoub stephentoub deleted the cookie_allocs branch March 29, 2016 15:46
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Reduce allocations in CookieContainer.GetCookieHeader

Commit migrated from dotnet/corefx@f22362d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants