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 #591

Closed

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Mar 20, 2016

  • ResponseCookies.Append() allocates lots, big lots #561
  • new SetCookieHeaderValue.AppendToStringBuilder() method; avoids per-call StringBuilder allocation
  • ResponseCookies uses ObjectPool<StringBuilder> that ResponseCookiesFeature provides
  • ResponseCookiesFeature creates an ObjectPoolProvider instance if none in DI
  • IResponseCookiesFeature exposes int properties supporting middleware overrides of the pool policy

nit: Add some doc comments

@dougbu
Copy link
Contributor Author

dougbu commented Mar 20, 2016

Note this is based on #589

/cc @davidfowl here's one idea for addressing #561. This does not require any changes in other repos. Middleware can adjust the pool policies but I doubt our middleware needs that.

Details

Looking more closely at the repos that add cookies, I suspect most, even Antiforgery, will always add cookie values shorter than the 4k default maximum size used here. In particular, the Antiforgery cookie token is shorter than I thought because it does not contain claims and most other token values are short identifiers. In addition CookieOptions shouldn't make cookie values significantly longer.

Probably worth measuring allocations when using the TwitterMiddleware. The TwitterHandler uses a RequestToken containing a fair amount of information. But it appears that cookie is a session cookie. (So making the test realistic may be difficult 😈)

@davidfowl
Copy link
Member

This still assumes something is coming from services no?

@dougbu
Copy link
Contributor Author

dougbu commented Mar 20, 2016

This still assumes something is coming from services no?

No. If an ObjectPoolProvider is a registered service, the feature uses it. But it'll fall back to creating its own DefaultObjectPoolProvider otherwise.

@dougbu dougbu force-pushed the dougbu/feature.interfaces.561 branch from ad9bcb0 to 6f24508 Compare March 21, 2016 16:32
… cookies

- #561
- new `SetCookieHeaderValue.AppendToStringBuilder()` method; avoids per-call `StringBuilder` allocation
- `ResponseCookies` uses `ObjectPool<StringBuilder>` that `ResponseCookiesFeature` provides
- `ResponseCookiesFeature` creates an `ObjectPoolProvider` instance if none in DI
- `IResponseCookiesFeature` exposes `int` properties supporting middleware overrides of the pool policy

nit: Add some doc comments
@dougbu dougbu force-pushed the dougbu/cookie.maker.561.II branch from 60208b3 to b5b204c Compare March 21, 2016 16:33
/// For example, the initial capacity of <see cref="System.Text.StringBuilder"/> instances obtained
/// from an object pool.
/// </remarks>
int InitialPooledInstanceCapacity { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

👎 This is an implementation detail, not a generic feature property.

@dougbu
Copy link
Contributor Author

dougbu commented Mar 21, 2016

Closing. Will reopen against dev.

@dougbu dougbu closed this Mar 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants