Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Use pooled StringBuilder to reduce allocations when adding response cookies #587

Closed
wants to merge 2 commits into from
Closed

Use pooled StringBuilder to reduce allocations when adding response cookies #587

wants to merge 2 commits into from

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Mar 15, 2016

@dougbu
Copy link
Contributor Author

dougbu commented Mar 15, 2016

/cc @Tratcher

@dougbu dougbu mentioned this pull request Mar 15, 2016
Closed
Tratcher
Tratcher reviewed Mar 15, 2016
View reviewed changes
src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs
/// The <see cref="StringBuilder"/> to receive the string representation of this
/// <see cref="SetCookieHeaderValue"/>.
/// </param>
public void AddStringValue(StringBuilder builder)
Copy link
Member

Choose a reason for hiding this comment

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

This name is confusing. ToStringBuilder? CopyToStringBuilder? SerializeToStringBuilder? AppendToStringBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let's go w/ AppendToStringBuilder.

@Tratcher
Copy link
Member

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.CookiePolicy/CookiePolicyMiddleware.cs#L29 will also need to be updated.

Looks OK. Check with @davidfowl on the new hosting service.

@dougbu
Copy link
Contributor Author

dougbu commented Mar 16, 2016

CookiePolicyMiddleware's ResponseCookieFeature instantiation will also need to be updated

That line is fine because ResponseCookieFeature has the same constructor it did before. I've successfully built aspnet/KestrelHttpServer and aspnet/Security with this and aspnet/Hosting#652 in place.

@dougbu
Copy link
Contributor Author

dougbu commented Mar 16, 2016

When using this branch and aspnet/Hosting#652, overall allocations go down approximately 1.7%. Big savings in char[] and StringBuilder instances:

Before

dev allocations

After

maker allocations

maker builders

@dougbu
Copy link
Contributor Author

dougbu commented Mar 16, 2016

Will come back to the pooling once we've got a few other ducks in line.

@dougbu dougbu closed this Mar 16, 2016
@dougbu dougbu deleted the dougbu/cookie.maker.561 branch March 19, 2016 19:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dougbu @Tratcher @dnfclas