From 4c3a3ea3853813f0c3f96da3678b2971ba573380 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Tue, 15 Mar 2016 15:53:20 -0700 Subject: [PATCH 1/2] Use pooled `StringBuilder` to reduce allocations when adding response cookies - #561 --- .../Features/ResponseCookiesFeature.cs | 14 ++++- .../ResponseCookies.cs | 55 +++++++++++++++---- src/Microsoft.AspNetCore.Http/project.json | 1 + .../SetCookieHeaderValue.cs | 33 +++++++---- .../ResponseCookiesTest.cs | 15 +++-- .../SetCookieHeaderValueTest.cs | 12 ++++ 6 files changed, 100 insertions(+), 30 deletions(-) diff --git a/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs b/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs index 136ecd2a..e56b7b11 100644 --- a/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs +++ b/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs @@ -1,22 +1,27 @@ // 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.Text; using Microsoft.AspNetCore.Http.Internal; +using Microsoft.Extensions.ObjectPool; namespace Microsoft.AspNetCore.Http.Features.Internal { public class ResponseCookiesFeature : IResponseCookiesFeature { private FeatureReferences _features; + private FeatureReferences _services; private IResponseCookies _cookiesCollection; public ResponseCookiesFeature(IFeatureCollection features) { _features = new FeatureReferences(features); + _services = new FeatureReferences(features); } - private IHttpResponseFeature HttpResponseFeature => - _features.Fetch(ref _features.Cache, f => null); + private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, f => null); + + private IServiceProvidersFeature ServiceProvidersFeature => _services.Fetch(ref _services.Cache, f => null); public IResponseCookies Cookies { @@ -25,8 +30,11 @@ public IResponseCookies Cookies if (_cookiesCollection == null) { var headers = HttpResponseFeature.Headers; - _cookiesCollection = new ResponseCookies(headers); + var serviceProvider = ServiceProvidersFeature.RequestServices; + var pool = (ObjectPool)serviceProvider.GetService(typeof(ObjectPool)); + _cookiesCollection = new ResponseCookies(headers, pool); } + return _cookiesCollection; } } diff --git a/src/Microsoft.AspNetCore.Http/ResponseCookies.cs b/src/Microsoft.AspNetCore.Http/ResponseCookies.cs index 16832c7d..f2cd5372 100644 --- a/src/Microsoft.AspNetCore.Http/ResponseCookies.cs +++ b/src/Microsoft.AspNetCore.Http/ResponseCookies.cs @@ -2,8 +2,9 @@ // 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; @@ -14,18 +15,26 @@ namespace Microsoft.AspNetCore.Http.Internal /// public class ResponseCookies : IResponseCookies { + private readonly ObjectPool _builderPool; + /// /// Create a new wrapper /// - /// - public ResponseCookies(IHeaderDictionary headers) + /// The for the response. + /// The used. + public ResponseCookies(IHeaderDictionary headers, ObjectPool builderPool) { if (headers == null) { throw new ArgumentNullException(nameof(headers)); } + if (builderPool == null) + { + throw new ArgumentNullException(nameof(builderPool)); + } Headers = headers; + _builderPool = builderPool; } private IHeaderDictionary Headers { get; set; } @@ -38,13 +47,25 @@ public ResponseCookies(IHeaderDictionary headers) 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; + var stringBuilder = _builderPool.Get(); + try + { + setCookieHeaderValue.AddStringValue(stringBuilder); + cookieValue = stringBuilder.ToString(); + } + finally + { + _builderPool.Return(stringBuilder); + } + + Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue); } /// @@ -61,8 +82,8 @@ public void Append(string key, string value, CookieOptions options) } var setCookieHeaderValue = new SetCookieHeaderValue( - Uri.EscapeDataString(key), - Uri.EscapeDataString(value)) + Uri.EscapeDataString(key), + Uri.EscapeDataString(value)) { Domain = options.Domain, Path = options.Path, @@ -71,7 +92,19 @@ public void Append(string key, string value, CookieOptions options) HttpOnly = options.HttpOnly, }; - Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], setCookieHeaderValue.ToString()); + string cookieValue; + var stringBuilder = _builderPool.Get(); + try + { + setCookieHeaderValue.AddStringValue(stringBuilder); + cookieValue = stringBuilder.ToString(); + } + finally + { + _builderPool.Return(stringBuilder); + } + + Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue); } /// @@ -94,7 +127,7 @@ public void Delete(string key, CookieOptions options) { throw new ArgumentNullException(nameof(options)); } - + var encodedKeyPlusEquals = Uri.EscapeDataString(key) + "="; bool domainHasValue = !string.IsNullOrEmpty(options.Domain); bool pathHasValue = !string.IsNullOrEmpty(options.Path); @@ -130,7 +163,7 @@ public void Delete(string key, CookieOptions options) 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 8e1578e2..0052abaf 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 37372e01..2d6d8421 100644 --- a/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs +++ b/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs @@ -90,42 +90,53 @@ public string Value public override string ToString() { StringBuilder header = new StringBuilder(); + AddStringValue(header); - header.Append(_name); - header.Append("="); - header.Append(_value); + return header.ToString(); + } + + /// + /// Add string representation of this to given . + /// + /// + /// The to receive the string representation of this + /// . + /// + public void AddStringValue(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/ResponseCookiesTest.cs b/test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs index cc26d657..5d7a73fc 100644 --- a/test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs +++ b/test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs @@ -1,19 +1,24 @@ // 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 { + private static readonly ObjectPool _builderPool = + new DefaultObjectPoolProvider().Create(new StringBuilderPooledObjectPolicy()); + [Fact] public void DeleteCookieShouldSetDefaultPath() { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var cookies = new ResponseCookies(headers, _builderPool); var testcookie = "TestCookie"; cookies.Delete(testcookie); @@ -29,7 +34,7 @@ public void DeleteCookieShouldSetDefaultPath() public void NoParamsDeleteRemovesCookieCreatedByAdd() { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var cookies = new ResponseCookies(headers, _builderPool); var testcookie = "TestCookie"; cookies.Append(testcookie, testcookie); @@ -49,7 +54,7 @@ public void NoParamsDeleteRemovesCookieCreatedByAdd() public void EscapesKeyValuesBeforeSettingCookie(string key, string value, 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 5d2c1385..b5758e85 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 @@ public void SetCookieHeaderValue_ToString(SetCookieHeaderValue input, string exp Assert.Equal(expectedValue, input.ToString()); } + [Theory] + [MemberData(nameof(SetCookieHeaderDataSet))] + public void SetCookieHeaderValue_AddStringValue(SetCookieHeaderValue input, string expectedValue) + { + var builder = new StringBuilder(); + + input.AddStringValue(builder); + + Assert.Equal(expectedValue, builder.ToString()); + } + [Theory] [MemberData(nameof(SetCookieHeaderDataSet))] public void SetCookieHeaderValue_Parse_AcceptsValidValues(SetCookieHeaderValue cookie, string expectedValue) From 5a94902d618397b3f8032a11df3fce9038eaeb1a Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Tue, 15 Mar 2016 16:25:43 -0700 Subject: [PATCH 2/2] PR comment: Rename to `AppendToStringBuilder()` --- src/Microsoft.AspNetCore.Http/ResponseCookies.cs | 4 ++-- src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs | 7 ++++--- .../SetCookieHeaderValueTest.cs | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.AspNetCore.Http/ResponseCookies.cs b/src/Microsoft.AspNetCore.Http/ResponseCookies.cs index f2cd5372..c374e24a 100644 --- a/src/Microsoft.AspNetCore.Http/ResponseCookies.cs +++ b/src/Microsoft.AspNetCore.Http/ResponseCookies.cs @@ -57,7 +57,7 @@ public void Append(string key, string value) var stringBuilder = _builderPool.Get(); try { - setCookieHeaderValue.AddStringValue(stringBuilder); + setCookieHeaderValue.AppendToStringBuilder(stringBuilder); cookieValue = stringBuilder.ToString(); } finally @@ -96,7 +96,7 @@ public void Append(string key, string value, CookieOptions options) var stringBuilder = _builderPool.Get(); try { - setCookieHeaderValue.AddStringValue(stringBuilder); + setCookieHeaderValue.AppendToStringBuilder(stringBuilder); cookieValue = stringBuilder.ToString(); } finally diff --git a/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs b/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs index 2d6d8421..7f790d66 100644 --- a/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs +++ b/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs @@ -90,19 +90,20 @@ public string Value public override string ToString() { StringBuilder header = new StringBuilder(); - AddStringValue(header); + AppendToStringBuilder(header); return header.ToString(); } /// - /// Add string representation of this to given . + /// Append string representation of this to given + /// . /// /// /// The to receive the string representation of this /// . /// - public void AddStringValue(StringBuilder builder) + public void AppendToStringBuilder(StringBuilder builder) { builder.Append(_name); builder.Append("="); diff --git a/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs b/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs index b5758e85..a3dad09a 100644 --- a/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs +++ b/test/Microsoft.Net.Http.Headers.Tests/SetCookieHeaderValueTest.cs @@ -267,11 +267,11 @@ public void SetCookieHeaderValue_ToString(SetCookieHeaderValue input, string exp [Theory] [MemberData(nameof(SetCookieHeaderDataSet))] - public void SetCookieHeaderValue_AddStringValue(SetCookieHeaderValue input, string expectedValue) + public void SetCookieHeaderValue_AppendToStringBuilder(SetCookieHeaderValue input, string expectedValue) { var builder = new StringBuilder(); - input.AddStringValue(builder); + input.AppendToStringBuilder(builder); Assert.Equal(expectedValue, builder.ToString()); }