From 80813f7c1e98384b8bbbcc445eca69448f6428ec Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Sat, 19 Mar 2016 16:45:26 -0700 Subject: [PATCH] Use pooled `StringBuilder` to reduce allocations when adding response cookies - #561 - new `SetCookieHeaderValue.AppendToStringBuilder()` method; avoids per-call `StringBuilder` allocation - `ResponseCookies` uses `ObjectPool` that `ResponseCookiesFeature` provides - `ResponseCookies` works fine if no `ObjectPoolProvider` is available - `IHttpContextFactory` instance is a singleton instantiated from CI - make `HttpContextFactory` `ObjectPoolProvider` and `ResponseCookiesFeature`-aware - apply same pattern to sample `PooledHttpContextFactory` - pool is not currently configurable; defaults are fine for response cookies - if we need (policy) configuration, would add an `IOptions` nit: Add some doc comments --- samples/SampleApp/PooledHttpContextFactory.cs | 26 +++++- .../IResponseCookies.cs | 29 +++--- .../IResponseCookiesFeature.cs | 6 ++ .../Features/ResponseCookiesFeature.cs | 44 ++++++++- .../HttpContextFactory.cs | 27 +++++- .../ResponseCookies.cs | 93 ++++++++++++------- src/Microsoft.AspNetCore.Http/project.json | 1 + .../SetCookieHeaderValue.cs | 34 ++++--- .../HttpContextFactoryTests.cs | 5 +- .../ResponseCookiesTest.cs | 64 ++++++++++--- .../SetCookieHeaderValueTest.cs | 12 +++ 11 files changed, 261 insertions(+), 80 deletions(-) diff --git a/samples/SampleApp/PooledHttpContextFactory.cs b/samples/SampleApp/PooledHttpContextFactory.cs index 6128e4ff49..ca22e01183 100644 --- a/samples/SampleApp/PooledHttpContextFactory.cs +++ b/samples/SampleApp/PooledHttpContextFactory.cs @@ -1,24 +1,48 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; +using System.Text; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Http.Features.Internal; +using Microsoft.Extensions.ObjectPool; namespace SampleApp { public class PooledHttpContextFactory : IHttpContextFactory { + private readonly ObjectPool _builderPool; private readonly IHttpContextAccessor _httpContextAccessor; private readonly Stack _pool = new Stack(); - public PooledHttpContextFactory(IHttpContextAccessor httpContextAccessor) + public PooledHttpContextFactory(ObjectPoolProvider poolProvider) + : this(poolProvider, httpContextAccessor: null) { + } + + public PooledHttpContextFactory(ObjectPoolProvider poolProvider, IHttpContextAccessor httpContextAccessor) + { + if (poolProvider == null) + { + throw new ArgumentNullException(nameof(poolProvider)); + } + + _builderPool = poolProvider.CreateStringBuilderPool(); _httpContextAccessor = httpContextAccessor; } public HttpContext Create(IFeatureCollection featureCollection) { + if (featureCollection == null) + { + throw new ArgumentNullException(nameof(featureCollection)); + } + + var responseCookiesFeature = new ResponseCookiesFeature(featureCollection, _builderPool); + featureCollection.Set(responseCookiesFeature); + PooledHttpContext httpContext = null; lock (_pool) { diff --git a/src/Microsoft.AspNetCore.Http.Features/IResponseCookies.cs b/src/Microsoft.AspNetCore.Http.Features/IResponseCookies.cs index dd8696f358..9c8c3b42ba 100644 --- a/src/Microsoft.AspNetCore.Http.Features/IResponseCookies.cs +++ b/src/Microsoft.AspNetCore.Http.Features/IResponseCookies.cs @@ -4,36 +4,39 @@ namespace Microsoft.AspNetCore.Http { /// - /// A wrapper for the response Set-Cookie header + /// A wrapper for the response Set-Cookie header. /// public interface IResponseCookies { /// - /// Add a new cookie and value + /// Add a new cookie and value. /// - /// - /// + /// Name of the new cookie. + /// Value of the new cookie. void Append(string key, string value); /// - /// Add a new cookie + /// Add a new cookie. /// - /// - /// - /// + /// Name of the new cookie. + /// Value of the new cookie. + /// included in the new cookie setting. void Append(string key, string value, CookieOptions options); /// - /// Sets an expired cookie + /// Sets an expired cookie. /// - /// + /// Name of the cookie to expire. void Delete(string key); /// - /// Sets an expired cookie + /// Sets an expired cookie. /// - /// - /// + /// Name of the cookie to expire. + /// + /// used to discriminate the particular cookie to expire. The + /// and values are especially important. + /// void Delete(string key, CookieOptions options); } } diff --git a/src/Microsoft.AspNetCore.Http.Features/IResponseCookiesFeature.cs b/src/Microsoft.AspNetCore.Http.Features/IResponseCookiesFeature.cs index fb5ea09258..7ce1041840 100644 --- a/src/Microsoft.AspNetCore.Http.Features/IResponseCookiesFeature.cs +++ b/src/Microsoft.AspNetCore.Http.Features/IResponseCookiesFeature.cs @@ -3,8 +3,14 @@ namespace Microsoft.AspNetCore.Http.Features { + /// + /// A helper for creating the response Set-Cookie header. + /// public interface IResponseCookiesFeature { + /// + /// Gets the wrapper for the response Set-Cookie header. + /// IResponseCookies Cookies { get; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs b/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs index 136ecd2a8e..9481ac0a8e 100644 --- a/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs +++ b/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs @@ -1,23 +1,58 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Text; using Microsoft.AspNetCore.Http.Internal; +using Microsoft.Extensions.ObjectPool; namespace Microsoft.AspNetCore.Http.Features.Internal { + /// + /// Default implementation of . + /// public class ResponseCookiesFeature : IResponseCookiesFeature { + // Object pool will be null only in test scenarios e.g. if code news up a DefaultHttpContext. + private readonly ObjectPool _builderPool; + private FeatureReferences _features; private IResponseCookies _cookiesCollection; + /// + /// Initializes a new instance. + /// + /// + /// containing all defined features, including this + /// and the . + /// public ResponseCookiesFeature(IFeatureCollection features) + : this(features, builderPool: null) { - _features = new FeatureReferences(features); } - private IHttpResponseFeature HttpResponseFeature => - _features.Fetch(ref _features.Cache, f => null); + /// + /// Initializes a new instance. + /// + /// + /// containing all defined features, including this + /// and the . + /// + /// The , if available. + public ResponseCookiesFeature(IFeatureCollection features, ObjectPool builderPool) + { + if (features == null) + { + throw new ArgumentNullException(nameof(features)); + } + _features = new FeatureReferences(features); + _builderPool = builderPool; + } + + private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, f => null); + + /// public IResponseCookies Cookies { get @@ -25,8 +60,9 @@ namespace Microsoft.AspNetCore.Http.Features.Internal if (_cookiesCollection == null) { var headers = HttpResponseFeature.Headers; - _cookiesCollection = new ResponseCookies(headers); + _cookiesCollection = new ResponseCookies(headers, _builderPool); } + return _cookiesCollection; } } diff --git a/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs b/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs index 2f6717641f..95efc68bf9 100644 --- a/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs +++ b/src/Microsoft.AspNetCore.Http/HttpContextFactory.cs @@ -1,30 +1,51 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Text; using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Http.Features.Internal; +using Microsoft.Extensions.ObjectPool; namespace Microsoft.AspNetCore.Http.Internal { public class HttpContextFactory : IHttpContextFactory { - private IHttpContextAccessor _httpContextAccessor; + private readonly ObjectPool _builderPool; + private readonly IHttpContextAccessor _httpContextAccessor; - public HttpContextFactory() : this(httpContextAccessor: null) + public HttpContextFactory(ObjectPoolProvider poolProvider) + : this(poolProvider, httpContextAccessor: null) { } - public HttpContextFactory(IHttpContextAccessor httpContextAccessor) + public HttpContextFactory(ObjectPoolProvider poolProvider, IHttpContextAccessor httpContextAccessor) { + if (poolProvider == null) + { + throw new ArgumentNullException(nameof(poolProvider)); + } + + _builderPool = poolProvider.CreateStringBuilderPool(); _httpContextAccessor = httpContextAccessor; } public HttpContext Create(IFeatureCollection featureCollection) { + if (featureCollection == null) + { + throw new ArgumentNullException(nameof(featureCollection)); + } + + var responseCookiesFeature = new ResponseCookiesFeature(featureCollection, _builderPool); + featureCollection.Set(responseCookiesFeature); + var httpContext = new DefaultHttpContext(featureCollection); if (_httpContextAccessor != null) { _httpContextAccessor.HttpContext = httpContext; } + return httpContext; } diff --git a/src/Microsoft.AspNetCore.Http/ResponseCookies.cs b/src/Microsoft.AspNetCore.Http/ResponseCookies.cs index 16832c7dbc..4a8538371b 100644 --- a/src/Microsoft.AspNetCore.Http/ResponseCookies.cs +++ b/src/Microsoft.AspNetCore.Http/ResponseCookies.cs @@ -2,23 +2,27 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Text.Encodings.Web; using System.Collections.Generic; +using System.Text; +using Microsoft.Extensions.ObjectPool; using Microsoft.Extensions.Primitives; using Microsoft.Net.Http.Headers; namespace Microsoft.AspNetCore.Http.Internal { /// - /// A wrapper for the response Set-Cookie header + /// A wrapper for the response Set-Cookie header. /// public class ResponseCookies : IResponseCookies { + private readonly ObjectPool _builderPool; + /// - /// Create a new wrapper + /// Create a new wrapper. /// - /// - public ResponseCookies(IHeaderDictionary headers) + /// The for the response. + /// The , if available. + public ResponseCookies(IHeaderDictionary headers, ObjectPool builderPool) { if (headers == null) { @@ -26,33 +30,44 @@ namespace Microsoft.AspNetCore.Http.Internal } Headers = headers; + _builderPool = builderPool; } private IHeaderDictionary Headers { get; set; } - /// - /// Add a new cookie and value - /// - /// - /// + /// public void Append(string key, string value) { var setCookieHeaderValue = new SetCookieHeaderValue( - Uri.EscapeDataString(key), - Uri.EscapeDataString(value)) + Uri.EscapeDataString(key), + Uri.EscapeDataString(value)) { Path = "/" }; - Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], setCookieHeaderValue.ToString()); + string cookieValue; + if (_builderPool == null) + { + cookieValue = setCookieHeaderValue.ToString(); + } + else + { + var stringBuilder = _builderPool.Get(); + try + { + setCookieHeaderValue.AppendToStringBuilder(stringBuilder); + cookieValue = stringBuilder.ToString(); + } + finally + { + _builderPool.Return(stringBuilder); + } + } + + Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue); } - /// - /// Add a new cookie - /// - /// - /// - /// + /// public void Append(string key, string value, CookieOptions options) { if (options == null) @@ -61,8 +76,8 @@ namespace Microsoft.AspNetCore.Http.Internal } var setCookieHeaderValue = new SetCookieHeaderValue( - Uri.EscapeDataString(key), - Uri.EscapeDataString(value)) + Uri.EscapeDataString(key), + Uri.EscapeDataString(value)) { Domain = options.Domain, Path = options.Path, @@ -71,30 +86,42 @@ namespace Microsoft.AspNetCore.Http.Internal HttpOnly = options.HttpOnly, }; - Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], setCookieHeaderValue.ToString()); + string cookieValue; + if (_builderPool == null) + { + cookieValue = setCookieHeaderValue.ToString(); + } + else + { + var stringBuilder = _builderPool.Get(); + try + { + setCookieHeaderValue.AppendToStringBuilder(stringBuilder); + cookieValue = stringBuilder.ToString(); + } + finally + { + _builderPool.Return(stringBuilder); + } + } + + Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue); } - /// - /// Sets an expired cookie - /// - /// + /// public void Delete(string key) { Delete(key, new CookieOptions() { Path = "/" }); } - /// - /// Sets an expired cookie - /// - /// - /// + /// public void Delete(string key, CookieOptions options) { if (options == null) { throw new ArgumentNullException(nameof(options)); } - + var encodedKeyPlusEquals = Uri.EscapeDataString(key) + "="; bool domainHasValue = !string.IsNullOrEmpty(options.Domain); bool pathHasValue = !string.IsNullOrEmpty(options.Path); @@ -130,7 +157,7 @@ namespace Microsoft.AspNetCore.Http.Internal newValues.Add(values[i]); } } - + Headers[HeaderNames.SetCookie] = new StringValues(newValues.ToArray()); } diff --git a/src/Microsoft.AspNetCore.Http/project.json b/src/Microsoft.AspNetCore.Http/project.json index 8e1578e21c..0052abaf91 100644 --- a/src/Microsoft.AspNetCore.Http/project.json +++ b/src/Microsoft.AspNetCore.Http/project.json @@ -17,6 +17,7 @@ "dependencies": { "Microsoft.AspNetCore.Http.Abstractions": "1.0.0-*", "Microsoft.AspNetCore.WebUtilities": "1.0.0-*", + "Microsoft.Extensions.ObjectPool": "1.0.0-*", "Microsoft.Net.Http.Headers": "1.0.0-*" }, "frameworks": { diff --git a/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs b/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs index 37372e013c..7f790d6617 100644 --- a/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs +++ b/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs @@ -90,42 +90,54 @@ namespace Microsoft.Net.Http.Headers public override string ToString() { StringBuilder header = new StringBuilder(); + AppendToStringBuilder(header); - header.Append(_name); - header.Append("="); - header.Append(_value); + return header.ToString(); + } + + /// + /// Append string representation of this to given + /// . + /// + /// + /// The to receive the string representation of this + /// . + /// + public void AppendToStringBuilder(StringBuilder builder) + { + builder.Append(_name); + builder.Append("="); + builder.Append(_value); if (Expires.HasValue) { - AppendSegment(header, ExpiresToken, HeaderUtilities.FormatDate(Expires.Value)); + AppendSegment(builder, ExpiresToken, HeaderUtilities.FormatDate(Expires.Value)); } if (MaxAge.HasValue) { - AppendSegment(header, MaxAgeToken, HeaderUtilities.FormatInt64((long)MaxAge.Value.TotalSeconds)); + AppendSegment(builder, MaxAgeToken, HeaderUtilities.FormatInt64((long)MaxAge.Value.TotalSeconds)); } if (Domain != null) { - AppendSegment(header, DomainToken, Domain); + AppendSegment(builder, DomainToken, Domain); } if (Path != null) { - AppendSegment(header, PathToken, Path); + AppendSegment(builder, PathToken, Path); } if (Secure) { - AppendSegment(header, SecureToken, null); + AppendSegment(builder, SecureToken, null); } if (HttpOnly) { - AppendSegment(header, HttpOnlyToken, null); + AppendSegment(builder, HttpOnlyToken, null); } - - return header.ToString(); } private static void AppendSegment(StringBuilder builder, string name, string value) diff --git a/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs b/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs index f44a567b24..29029c4ad8 100644 --- a/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs +++ b/test/Microsoft.AspNetCore.Http.Tests/HttpContextFactoryTests.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.AspNetCore.Http.Features; +using Microsoft.Extensions.ObjectPool; using Xunit; namespace Microsoft.AspNetCore.Http.Internal @@ -13,7 +14,7 @@ namespace Microsoft.AspNetCore.Http.Internal { // Arrange var accessor = new HttpContextAccessor(); - var contextFactory = new HttpContextFactory(accessor); + var contextFactory = new HttpContextFactory(new DefaultObjectPoolProvider(), accessor); // Act var context = contextFactory.Create(new FeatureCollection()); @@ -26,7 +27,7 @@ namespace Microsoft.AspNetCore.Http.Internal public void AllowsCreatingContextWithoutSettingAccessor() { // Arrange - var contextFactory = new HttpContextFactory(); + var contextFactory = new HttpContextFactory(new DefaultObjectPoolProvider()); // Act & Assert var context = contextFactory.Create(new FeatureCollection()); diff --git a/test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs b/test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs index cc26d657c7..021f517bd6 100644 --- a/test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs +++ b/test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs @@ -1,19 +1,37 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using Xunit; -using Microsoft.Net.Http.Headers; +using System.Text; using Microsoft.AspNetCore.Http.Internal; +using Microsoft.Extensions.ObjectPool; +using Microsoft.Net.Http.Headers; +using Xunit; namespace Microsoft.AspNetCore.Http.Tests { public class ResponseCookiesTest { - [Fact] - public void DeleteCookieShouldSetDefaultPath() + private static readonly ObjectPool _builderPool = + new DefaultObjectPoolProvider().Create(new StringBuilderPooledObjectPolicy()); + + public static TheoryData BuilderPoolData + { + get + { + return new TheoryData> + { + null, + _builderPool, + }; + } + } + + [Theory] + [MemberData(nameof(BuilderPoolData))] + public void DeleteCookieShouldSetDefaultPath(ObjectPool builderPool) { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var cookies = new ResponseCookies(headers, builderPool); var testcookie = "TestCookie"; cookies.Delete(testcookie); @@ -25,11 +43,12 @@ namespace Microsoft.AspNetCore.Http.Tests Assert.Contains("expires=Thu, 01 Jan 1970 00:00:00 GMT", cookieHeaderValues[0]); } - [Fact] - public void NoParamsDeleteRemovesCookieCreatedByAdd() + [Theory] + [MemberData(nameof(BuilderPoolData))] + public void NoParamsDeleteRemovesCookieCreatedByAdd(ObjectPool builderPool) { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var cookies = new ResponseCookies(headers, builderPool); var testcookie = "TestCookie"; cookies.Append(testcookie, testcookie); @@ -42,14 +61,33 @@ namespace Microsoft.AspNetCore.Http.Tests Assert.Contains("expires=Thu, 01 Jan 1970 00:00:00 GMT", cookieHeaderValues[0]); } + public static TheoryData EscapesKeyValuesBeforeSettingCookieData + { + get + { + // key, value, object pool, expected + return new TheoryData, string> + { + { "key", "value", null, "key=value" }, + { "key,", "!value", null, "key%2C=%21value" }, + { "ke#y,", "val^ue", null, "ke%23y%2C=val%5Eue" }, + { "key", "value", _builderPool, "key=value" }, + { "key,", "!value", _builderPool, "key%2C=%21value" }, + { "ke#y,", "val^ue", _builderPool, "ke%23y%2C=val%5Eue" }, + }; + } + } + [Theory] - [InlineData("key", "value", "key=value")] - [InlineData("key,", "!value", "key%2C=%21value")] - [InlineData("ke#y,", "val^ue", "ke%23y%2C=val%5Eue")] - public void EscapesKeyValuesBeforeSettingCookie(string key, string value, string expected) + [MemberData(nameof(EscapesKeyValuesBeforeSettingCookieData))] + public void EscapesKeyValuesBeforeSettingCookie( + string key, + string value, + ObjectPool builderPool, + string expected) { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var cookies = new ResponseCookies(headers, builderPool); cookies.Append(key, value); diff --git a/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs b/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs index 5d2c138558..a3dad09a24 100644 --- a/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs +++ b/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Text; using Xunit; namespace Microsoft.Net.Http.Headers @@ -264,6 +265,17 @@ namespace Microsoft.Net.Http.Headers Assert.Equal(expectedValue, input.ToString()); } + [Theory] + [MemberData(nameof(SetCookieHeaderDataSet))] + public void SetCookieHeaderValue_AppendToStringBuilder(SetCookieHeaderValue input, string expectedValue) + { + var builder = new StringBuilder(); + + input.AppendToStringBuilder(builder); + + Assert.Equal(expectedValue, builder.ToString()); + } + [Theory] [MemberData(nameof(SetCookieHeaderDataSet))] public void SetCookieHeaderValue_Parse_AcceptsValidValues(SetCookieHeaderValue cookie, string expectedValue)