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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion samples/SampleApp/PooledHttpContextFactory.cs
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't need this addition once I'm done #592.

using Microsoft.Extensions.ObjectPool;

namespace SampleApp
{
public class PooledHttpContextFactory : IHttpContextFactory
{
private readonly ObjectPool<StringBuilder> _builderPool;
private readonly IHttpContextAccessor _httpContextAccessor;
private readonly Stack<PooledHttpContext> _pool = new Stack<PooledHttpContext>();

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<IResponseCookiesFeature>(responseCookiesFeature);

PooledHttpContext httpContext = null;
lock (_pool)
{
Expand Down
29 changes: 16 additions & 13 deletions src/Microsoft.AspNetCore.Http.Features/IResponseCookies.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,36 +4,39 @@
namespace Microsoft.AspNetCore.Http
{
/// <summary>
/// A wrapper for the response Set-Cookie header
/// A wrapper for the response Set-Cookie header.
/// </summary>
public interface IResponseCookies
{
/// <summary>
/// Add a new cookie and value
/// Add a new cookie and value.
/// </summary>
/// <param name="key"></param>
/// <param name="value"></param>
/// <param name="key">Name of the new cookie.</param>
/// <param name="value">Value of the new cookie.</param>
void Append(string key, string value);

/// <summary>
/// Add a new cookie
/// Add a new cookie.
/// </summary>
/// <param name="key"></param>
/// <param name="value"></param>
/// <param name="options"></param>
/// <param name="key">Name of the new cookie.</param>
/// <param name="value">Value of the new cookie.</param>
/// <param name="options"><see cref="CookieOptions"/> included in the new cookie setting.</param>
void Append(string key, string value, CookieOptions options);

/// <summary>
/// Sets an expired cookie
/// Sets an expired cookie.
/// </summary>
/// <param name="key"></param>
/// <param name="key">Name of the cookie to expire.</param>
void Delete(string key);

/// <summary>
/// Sets an expired cookie
/// Sets an expired cookie.
/// </summary>
/// <param name="key"></param>
/// <param name="options"></param>
/// <param name="key">Name of the cookie to expire.</param>
/// <param name="options">
/// <see cref="CookieOptions"/> used to discriminate the particular cookie to expire. The
/// <see cref="CookieOptions.Domain"/> and <see cref="CookieOptions.Path"/> values are especially important.
/// </param>
void Delete(string key, CookieOptions options);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,14 @@

namespace Microsoft.AspNetCore.Http.Features
{
/// <summary>
/// A helper for creating the response Set-Cookie header.
/// </summary>
public interface IResponseCookiesFeature
{
/// <summary>
/// Gets the wrapper for the response Set-Cookie header.
/// </summary>
IResponseCookies Cookies { get; }
}
}
2 changes: 1 addition & 1 deletion src/Microsoft.AspNetCore.Http.Features/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"System.Net.WebSockets": "4.0.0-*",
"System.Runtime.Extensions": "4.1.0-*",
"System.Security.Claims": "4.0.1-*",
"System.Security.Cryptography.X509Certificates": "4.1.0-*",
"System.Security.Cryptography.X509Certificates": "4.0.0-*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure this will be necessary next time CI updates the feed. @pranavkm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, the 4.1.0 package is available in the new corefx build.

"System.Security.Principal": "4.0.1-*"
},
"imports": [
Expand Down
42 changes: 39 additions & 3 deletions src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs
Original file line number Diff line number Diff line change
@@ -1,32 +1,68 @@
// 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
{
/// <summary>
/// Default implementation of <see cref="IResponseCookiesFeature"/>.
/// </summary>
public class ResponseCookiesFeature : IResponseCookiesFeature
{
// Object pool will be null only in test scenarios e.g. if code news up a DefaultHttpContext.
private readonly ObjectPool<StringBuilder> _builderPool;

private FeatureReferences<IHttpResponseFeature> _features;
private IResponseCookies _cookiesCollection;

/// <summary>
/// Initializes a new <see cref="ResponseCookiesFeature"/> instance.
/// </summary>
/// <param name="features">
/// <see cref="IFeatureCollection"/> containing all defined features, including this
/// <see cref="IResponseCookiesFeature"/> and the <see cref="IHttpResponseFeature"/>.
/// </param>
public ResponseCookiesFeature(IFeatureCollection features)
: this(features, builderPool: null)
{
}

/// <summary>
/// Initializes a new <see cref="ResponseCookiesFeature"/> instance.
/// </summary>
/// <param name="features">
/// <see cref="IFeatureCollection"/> containing all defined features, including this
/// <see cref="IResponseCookiesFeature"/> and the <see cref="IHttpResponseFeature"/>.
/// </param>
/// <param name="builderPool">The <see cref="ObjectPool{T}"/>, if available.</param>
public ResponseCookiesFeature(IFeatureCollection features, ObjectPool<StringBuilder> builderPool)
{
if (features == null)
{
throw new ArgumentNullException(nameof(features));
}

_features = new FeatureReferences<IHttpResponseFeature>(features);
_builderPool = builderPool;
}

private IHttpResponseFeature HttpResponseFeature =>
_features.Fetch(ref _features.Cache, f => null);
private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, f => null);

/// <inheritdoc />
public IResponseCookies Cookies
{
get
{
if (_cookiesCollection == null)
{
var headers = HttpResponseFeature.Headers;
_cookiesCollection = new ResponseCookies(headers);
_cookiesCollection = new ResponseCookies(headers, _builderPool);
}

return _cookiesCollection;
}
}
Expand Down
27 changes: 24 additions & 3 deletions src/Microsoft.AspNetCore.Http/HttpContextFactory.cs
Original file line number Diff line number Diff line change
@@ -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
Copy link

Choose a reason for hiding this comment

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

Wasn't the plan to move the Features out of Internal namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I have a second wave of breaking changes (after moving the feature implementations) queued up. That'll cover both #549 and #592.

{
public class HttpContextFactory : IHttpContextFactory
{
private IHttpContextAccessor _httpContextAccessor;
private readonly ObjectPool<StringBuilder> _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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not testing constructor overloads w/ a non-null provider b/c that requires something ugly like passing a Mock<IFeatureCollection> to Create() (test project does not currently use Moq) or exposing an otherwise-useless property on the ResponseCookiesFeature.

{
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<IResponseCookiesFeature>(responseCookiesFeature);

var httpContext = new DefaultHttpContext(featureCollection);
if (_httpContextAccessor != null)
{
_httpContextAccessor.HttpContext = httpContext;
}

return httpContext;
}

Expand Down
Loading