ChunkingCookieManager could be optimized for perf and fewer allocations

[pranavkm]

• 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.
• DeleteCookie also could the delegate allocation using Enumerable.Where and use a bespoke iterator similar to ResponseCookies.Delete:
  • https://github.com/dotnet/aspnetcore/blob/main/src/Shared/ChunkingCookieManager/ChunkingCookieManager.cs#L282
  • https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/Internal/ResponseCookies.cs#L128-L142
• 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

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[uabarahona]

@pranavkm @davidfowl Sounds interesting, I would like to take this one, some questions:

• Any common place where I can put some perf tests to see if my refactors do have real gains?
• I am thinking a few strategies to follow in this refactor, but I do not loose nothing asking, do you have any docs when it comes to refactor something with perf as a goal?

[uabarahona]

@davidfowl @pranavkm I have send a new PR adding some performance refactoring's that follows the guidance of the original comment, there are pretty cool wins with it although it adds a new public api which will need to be reviewed.

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

[Tratcher]

Notes for the API review:
Could this IEnumerable or ICollection instead of IDictionary? I think the implementations would be more efficient with a List than a Dictionary. Random access isn't needed.
void Append(ICollection<KeyValuePair<<string, string>> keyValuePairs, CookieOptions options);

[davidfowl]

I think we should add overloads as appropriate to avoid transforming source data. If the caller has a dictionary then we need to make that work as is.

[uabarahona]

I have tested it with IEnumerable<KeyValuePair<<string, string>> and it actually improved the performance numbers and saved us 1k of allocation on DeleteCookies, I liked the idea as users can still pass IDictionary<string,string> without problems.

[davidfowl]

Excellent work @barahonajm! I hope to see more contributions like this from you