Optimizing ChunkingCookieManager #31625

uabarahona · 2021-04-09T00:49:22Z

Summary

These changes are to improve the performance for the operation done by ChunkingCookieManager

AppendCookies (~2x win)

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
AppendCookiesBefore	5.456 μs	0.1469 μs	0.4261 μs	183,279.4	0.1068	0.0076	-	4 KB
AppendCookiesAfter	2.442 μs	0.0284 μs	0.0237 μs	447,691.3	0.0801	-	-	3 KB

DeleteCookies (~4x win)

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
DeleteCookiesBefore	12.89 μs	0.255 μs	0.582 μs	77,597.0	0.3204	-	-	11 KB
DeleteCookiesAfter	3.773 μs	0.0504 μs	0.0421 μs	257,194.2	0.1373	-	-	4 KB

Details

This PR took as a guidance the original comments done on ticket #31508

1. DeleteCookie calls AppendCookie which is designed for larger chunked cookies. However, when deleting each of the values is an empty string. It could do something much more targeted.

It now calls directly a new public api on ResponseCookies.cs who makes sure to add a collection of key pair values as cookies to the Set-Cookier header in an efficient manner

2. DeleteCookie also could the delegate allocation using Enumerable.Where and use a bespoke iterator similar to ResponseCookies.Delete

It now deletes the response cookies using a simple for loop as ResponseCookies class does

3. AppendResponseCookie invokes ResponseCookies.Append which allocates a ResponseCookieValues instance and an intermediary StringValues for the header. Since we know we're running inside a loop, it could be better optimized for fewer allocations

It now calls directly a new public api on ResponseCookies.cs who makes sure to add a collection of key pair values as cookies to the Set-Cookier header in an efficient manner

New API Proposal

void Append(IDictionary<string, string> keyValuePairs, CookieOptions options);

As seen on ChunkingCookieManager the need to add several cookies at once using the same CookieOptions seems to be common, the new overload will cover this need by receiving a IDictionary<string, string> whose values will be added to the Set-Cookier response header in an efficient manner:

Allowing us to instante only one CookieOptions per all cookies added
Allowing us to instante only one SetCookieHeaderValue and be reused for all Cookies to be added
Prevent multiple StringValues.Concat

Addresses #31508

Update 1: Numbers were updated according to the recent changes based on the feedback

davidfowl · 2021-04-09T00:56:45Z

src/Http/Http/src/Internal/ResponseCookies.cs

+            {
+                if (_logger == null)
+                {
+                    var services = _features.Get<Features.IServiceProvidersFeature>()?.RequestServices;


We want to log from these components like this.

Thanks for the review, I kind of feel there is something missing after "like this", could you please tell me if I am missing something here?

@davidfowl this was copied from the existing code above, we can discuss it separately from this PR.

aspnetcore/src/Http/Http/src/Internal/ResponseCookies.cs

Lines 59 to 71 in 00b551e

if (!options.Secure && options.SameSite == SameSiteMode.None)

{

if (_logger == null)

{

var services = _features.Get<Features.IServiceProvidersFeature>()?.RequestServices;

_logger = services?.GetService<ILogger<ResponseCookies>>();

}

if (_logger != null)

{

Log.SameSiteCookieNotSecure(_logger, key);

}

}

Correct I did not fully look at this yet and just copied from the original append but I will definitely look if this can be improved too, thanks

src/Http/Http.Features/src/IResponseCookies.cs

src/Http/Http/src/Internal/ResponseCookies.cs

pranavkm · 2021-04-09T16:32:27Z

src/Http/Http/src/Internal/ResponseCookies.cs

+            };
+
+            var cookierHeaderValue = setCookieHeaderValue.ToString()[1..];
+            var cookies = new string[keyValuePairs.Count];


I think you could borrow this from the ArrayPool. Alternatively you could simply call Append n times. It's effectively what the IHeaderDictionary.Append(IList<T>) seems to do:

aspnetcore/src/Http/Http.Extensions/src/HeaderDictionaryTypeExtensions.cs

Lines 139 to 144 in 5b3ba86

var newValues = new string[values.Count];

for (var i = 0; i < values.Count; i++)

{

newValues[i] = values[i]!.ToString()!;

}

Headers.Append(name, new StringValues(newValues));

In respect to ArrayPool:

It sounded cool so I tried it using ArrayPool<string>.Shared.Rent(keyValuePairs.Count()) but it actually increases the mean about 100ns-200ns.

In respect to Headers.Append:

aspnetcore/src/Http/Http/src/Internal/ResponseCookies.cs

Lines 133 to 139 in 9a55381

foreach (var keyValuePair in keyValuePairs)

{

cookies[position] = string.Concat(_enableCookieNameEncoding ? Uri.EscapeDataString(keyValuePair.Key) : keyValuePair.Key, "=", Uri.EscapeDataString(keyValuePair.Value), cookierHeaderValue);

position++;

}

Headers.Append(HeaderNames.SetCookie, cookies);

I am indeed calling it but just once at the end (L139), I just wanted to avoid calling Headers.Append multiple times because at the end it calls StringValues.concat and it means unnecessary allocation of arrays, hopefully I was able to build the full array of new cookies just like the example you gave me.

aspnetcore/src/Http/Http.Abstractions/src/Internal/ParsingHelpers.cs

Lines 160 to 161 in c925f99

var existing = GetHeaderUnmodified(headers, key);

SetHeaderUnmodified(headers, key, StringValues.Concat(existing, values));

Thanks!

src/Http/Http/src/Internal/ResponseCookies.cs

src/Http/Http.Features/src/IResponseCookies.cs

pranavkm · 2021-04-09T19:04:50Z

@JuanShares looks like there's some interesting API design questions that this PR would take. Instead of randomizing you, could we discuss this as part of API review meeting and get back to you on a concrete proposal? That way we're not randomizing you too much

ghost · 2021-04-09T19:05:03Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

uabarahona · 2021-04-09T19:29:29Z

Cool! thank you all for the feedback, @pranavkm definitely! I will wait for the concrete proposal for the API in the meanwhile will address other comments :D

davidfowl · 2021-04-10T04:04:45Z

FWIW, I'm not a fan of the IEnumerable<T> here because it results in allocations on enumeration.

pranavkm · 2021-04-12T18:07:45Z

@barahonajm we think this needs some prep work on our end to allow using default interface implementations. We're going to track this as part of #31723. This PR should be unblocked once that is resolved.

@BrennanConroy / @davidfowl could we prioritize that issue to unblock this PR?

Tratcher · 2021-04-12T18:42:37Z

FWIW, I'm not a fan of the IEnumerable<T> here because it results in allocations on enumeration.

@davidfowl IList is your recommendation to avoid this?

davidfowl · 2021-04-12T18:44:11Z

@davidfowl IList is your recommendation to avoid this?

IReadOnlyList<T> yeah

Tratcher · 2021-04-12T18:44:22Z

@pranavkm I'll work with @barahonajm on this PR in parallel, we should be able to make progress using DIMs and some temporary #if NET statements.

davidfowl · 2021-04-12T19:23:38Z

Hmm now I want to try the ReadOnlySpan instead of IReadOnlyLisr

uabarahona · 2021-04-12T19:43:05Z

Well initially I went through the IEnumerable<KeyValuePair<string, string>> path because users could easily pass a Dictionary in that place, but now I believe you want a ReadOnlySpan as the main implementation used on ChunkingCookierManager and additionally add some overloads (if yes which overloads should there be?) for users to consume right?

davidfowl · 2021-04-12T19:48:26Z

Well initially I went through the IEnumerable<KeyValuePair<string, string>> path because users could easily pass a Dictionary in that place, but now I believe you want a ReadOnlySpan as the main implementation used on ChunkingCookierManager and additionally add some overloads (if yes which overloads should there be?) for users to consume right?

Can you humor me and try ReadOnlySpan<KeyValuePair<string, string>> instead of IReadOnlyList

uabarahona · 2021-04-12T20:24:19Z

Yes sir, I have tried, and these are the new results (spoiler: nice suggestion!)

AppendCookies (~2x win, 2x less allocations)

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
AppendCookiesBefore	5.456 μs	0.1469 μs	0.4261 μs	183,279.4	0.1068	0.0076	-	4 KB
AppendCookiesAfter[with IEnumerable]	2.442 μs	0.0284 μs	0.0237 μs	447,691.3	0.0801	-	-	3 KB
AppendCookiesAfter[with ReadOnlySpan]	2.177 us	0.0433 us	0.1125 us	459,285.3	0.0725	0.0038	-	2 KB

DeleteCookies (~4x win, ~3x less allocations)

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
DeleteCookiesBefore	12.89 μs	0.255 μs	0.582 μs	77,597.0	0.3204	-	-	11 KB
DeleteCookiesAfter[with IEnumerable]	3.773 μs	0.0504 μs	0.0421 μs	257,194.2	0.1373	-	-	4 KB
DeleteCookies[with ReadOnlySpan]	3.155 us	0.0625 us	0.0973 us	316,973.5	0.1183	-	-	4 KB

davidfowl · 2021-04-12T20:26:12Z

This is the new API 😄

uabarahona · 2021-04-17T01:13:29Z

@Tratcher Sorry for the delay, I finally was able to get back to this, I was thinking on the DIM implementation through these days and came to the conclusion it should be as simple as calling the normal Append are you agree with this? (This is my first use of DIM :D)

The other option is put a bit more work on this and bring the necessary stuff to the interface for the new method to work, if this is better, I can do it too.

aspnetcore/src/Http/Http.Features/src/IResponseCookies.cs

Lines 31 to 44 in df9ee2f

    
           #if NET6_0 
        
                   /// <summary> 
        
                   /// Add elements of specified dictionary as cookies. 
        
                   /// </summary> 
        
                   /// <param name="keyValuePairs">Key value pair collections whose elements will be added as cookies.</param> 
        
                   /// <param name="options"><see cref="CookieOptions"/> included in new cookie settings.</param> 
        
                   void Append(ReadOnlySpan<KeyValuePair<string, string>> keyValuePairs, CookieOptions options) 
        
                   { 
        
                       foreach (var keyValuePair in keyValuePairs) 
        
                       { 
        
                           Append(keyValuePair.Key, keyValuePair.Value, options); 
        
                       } 
        
                   } 
        
           #endif

FYI @davidfowl @pranavkm

Tratcher

The functional part of this looks fine, I've added some minor comments. I'll follow up about the PublicAPI cross targeting issues to see if this is the expected pattern. Worst case we get someone to do #31723 first and it becomes moot.

src/Http/Http/src/Internal/ResponseCookies.cs

src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs

src/Security/CookiePolicy/test/CookieChunkingTests.cs

src/Security/perf/Microbenchmarks/ChunkingCookieManagerBenchmark.cs

src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj

dougbu

From a build perspective but please address https://github.com/dotnet/aspnetcore/pull/31625/files?file-filters%5B%5D=.csproj&file-filters%5B%5D=.txt#r616226305 and get additional approvals before squish / merge.

src/Security/perf/Microbenchmarks/Microsoft.AspNetCore.Security.Microbenchmarks.csproj

src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj

src/Http/Http.Features/src/IResponseCookies.cs

ghost · 2021-04-20T16:13:59Z

Hello @Tratcher!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Tratcher · 2021-04-20T19:27:35Z

Thanks, that had a lot of build complications to deal with.

Adding initial refactor to improve perfromance on ChunkingCookieManager

6157a0b

uabarahona requested review from jkotalik, Tratcher and a team as code owners April 9, 2021 00:49

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 9, 2021

davidfowl reviewed Apr 9, 2021

View reviewed changes

src/Http/Http.Features/src/IResponseCookies.cs Outdated Show resolved Hide resolved

uabarahona added 2 commits April 8, 2021 21:17

Moving new API to correct file

7922591

Adding some interface implementation for tests

5f811cd

uabarahona requested review from javiercn and pranavkm as code owners April 9, 2021 01:59

pranavkm reviewed Apr 9, 2021

View reviewed changes

jkotalik reviewed Apr 9, 2021

View reviewed changes

src/Http/Http/src/Internal/ResponseCookies.cs Outdated Show resolved Hide resolved

Pilchie added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-runtime area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer labels Apr 9, 2021

Tratcher reviewed Apr 9, 2021

View reviewed changes

src/Http/Http.Features/src/IResponseCookies.cs Outdated Show resolved Hide resolved

pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Apr 9, 2021

BrennanConroy assigned Tratcher Apr 9, 2021

uabarahona added 2 commits April 9, 2021 20:24

Addressing feedback given by asp.net team

57a9b65

Remove spaces

9a55381

uabarahona closed this Apr 12, 2021

uabarahona added 3 commits April 12, 2021 16:28

Changing to use readonlyspan

2a2d15a

Adding DIM as discussion point

df9ee2f

CollectionMarshal

7710230

uabarahona added 2 commits April 17, 2021 21:30

Removing unnecesary implementations and fixing public api

ffb399b

Fixing public api for different targets

3a11950

pranavkm added this to the 6.0-preview5 milestone Apr 19, 2021

Tratcher reviewed Apr 19, 2021

View reviewed changes

dougbu reviewed Apr 19, 2021

View reviewed changes

src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj Outdated Show resolved Hide resolved

dougbu approved these changes Apr 20, 2021

View reviewed changes

src/Security/perf/Microbenchmarks/Microsoft.AspNetCore.Security.Microbenchmarks.csproj Outdated Show resolved Hide resolved

uabarahona commented Apr 20, 2021

View reviewed changes

src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj Outdated Show resolved Hide resolved

Addressing feedback from asp.net team

ab3e803

uabarahona force-pushed the perf/improve-performance-ChunkingCookieManager branch from ead98aa to ab3e803 Compare April 20, 2021 14:36

Tratcher approved these changes Apr 20, 2021

View reviewed changes

src/Http/Http.Features/src/IResponseCookies.cs Outdated Show resolved Hide resolved

Update src/Http/Http.Features/src/IResponseCookies.cs

d312bc9

Tratcher added the auto-merge label Apr 20, 2021

ghost merged commit 3c228e5 into dotnet:main Apr 20, 2021

uabarahona deleted the perf/improve-performance-ChunkingCookieManager branch April 20, 2021 19:53

Tratcher mentioned this pull request Apr 21, 2021

Move IFeatureCollection to new Extensions.Features assembly #32043

Merged

Tratcher added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 26, 2021

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023

This pull request was closed.

	if (!options.Secure && options.SameSite == SameSiteMode.None)
	{
	if (_logger == null)
	{
	var services = _features.Get<Features.IServiceProvidersFeature>()?.RequestServices;
	_logger = services?.GetService<ILogger<ResponseCookies>>();
	}

	if (_logger != null)
	{
	Log.SameSiteCookieNotSecure(_logger, key);
	}
	}

	var newValues = new string[values.Count];
	for (var i = 0; i < values.Count; i++)
	{
	newValues[i] = values[i]!.ToString()!;
	}
	Headers.Append(name, new StringValues(newValues));

	foreach (var keyValuePair in keyValuePairs)
	{
	cookies[position] = string.Concat(_enableCookieNameEncoding ? Uri.EscapeDataString(keyValuePair.Key) : keyValuePair.Key, "=", Uri.EscapeDataString(keyValuePair.Value), cookierHeaderValue);
	position++;
	}

	Headers.Append(HeaderNames.SetCookie, cookies);

	var existing = GetHeaderUnmodified(headers, key);
	SetHeaderUnmodified(headers, key, StringValues.Concat(existing, values));

Optimizing ChunkingCookieManager #31625

Optimizing ChunkingCookieManager #31625

Uh oh!

Conversation

uabarahona commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

AppendCookies (~2x win)

DeleteCookies (~4x win)

Details

New API Proposal

Uh oh!

davidfowl Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

uabarahona Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

Tratcher Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

uabarahona Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavkm Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

uabarahona Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavkm commented Apr 9, 2021

Uh oh!

ghost commented Apr 9, 2021

Uh oh!

uabarahona commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Apr 10, 2021

Uh oh!

pranavkm commented Apr 12, 2021

Uh oh!

Tratcher commented Apr 12, 2021

Uh oh!

davidfowl commented Apr 12, 2021

Uh oh!

Tratcher commented Apr 12, 2021

Uh oh!

davidfowl commented Apr 12, 2021

Uh oh!

uabarahona commented Apr 12, 2021

Uh oh!

davidfowl commented Apr 12, 2021

Uh oh!

uabarahona commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AppendCookies (~2x win, 2x less allocations)

DeleteCookies (~4x win, ~3x less allocations)

Uh oh!

davidfowl commented Apr 12, 2021

Uh oh!

uabarahona commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uabarahona commented Apr 9, 2021 •

edited

Loading

uabarahona Apr 10, 2021 •

edited

Loading

uabarahona Apr 10, 2021 •

edited

Loading

uabarahona commented Apr 9, 2021 •

edited

Loading

uabarahona commented Apr 12, 2021 •

edited

Loading

uabarahona commented Apr 17, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.