From 067eb9c6f80cd2e50fdf3b3d1af0fbd94ac7bc33 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Wed, 28 Sep 2016 19:35:04 +0100 Subject: [PATCH] Make FeatureReferences.Fetch inlineable (#704) --- .../FeatureReferences.cs | 60 +++++++++++++++---- .../DefaultAuthenticationManager.cs | 5 +- .../DefaultHttpContext.cs | 23 ++++--- .../Features/QueryFeature.cs | 5 +- .../Features/RequestCookiesFeature.cs | 5 +- .../Features/ResponseCookiesFeature.cs | 5 +- .../Internal/DefaultConnectionInfo.cs | 9 ++- .../Internal/DefaultHttpRequest.cs | 14 +++-- .../Internal/DefaultHttpResponse.cs | 8 ++- .../Internal/DefaultWebSocketManager.cs | 8 ++- 10 files changed, 108 insertions(+), 34 deletions(-) diff --git a/src/Microsoft.AspNetCore.Http.Features/FeatureReferences.cs b/src/Microsoft.AspNetCore.Http.Features/FeatureReferences.cs index adc10b3add..38bd2ec27a 100644 --- a/src/Microsoft.AspNetCore.Http.Features/FeatureReferences.cs +++ b/src/Microsoft.AspNetCore.Http.Features/FeatureReferences.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Runtime.CompilerServices; namespace Microsoft.AspNetCore.Http.Features { @@ -21,39 +22,72 @@ namespace Microsoft.AspNetCore.Http.Features // be able to pass ref values that "dot through" the TCache struct memory, // if it was a Property then that getter would return a copy of the memory // preventing the use of "ref" - public TCache Cache; + public TCache Cache; + // Careful with modifications to the Fetch method; it is carefully constructed for inlining + // See: https://github.com/aspnet/HttpAbstractions/pull/704 + // This method is 59 IL bytes and at inline call depth 3 from accessing a property. + // This combination is enough for the jit to consider it an "unprofitable inline" + // Aggressively inlining it causes the entire call chain to dissolve: + // + // This means this call graph: + // + // HttpResponse.Headers -> Response.HttpResponseFeature -> Fetch -> Fetch -> Revision + // -> Collection -> Collection + // -> Collection.Revision + // Has 6 calls eliminated and becomes just: -> UpdateCached + // + // HttpResponse.Headers -> Collection.Revision + // -> UpdateCached (not called on fast path) + // + // As this is inlined at the callsite we want to keep the method small, so it only detects + // if a reset or update is required and all the reset and update logic is pushed to UpdateCached. + // + // Generally Fetch is called at a ratio > x4 of UpdateCached so this is a large gain + [MethodImpl(MethodImplOptions.AggressiveInlining)] public TFeature Fetch( ref TFeature cached, TState state, Func factory) where TFeature : class { + var flush = false; var revision = Collection.Revision; - if (Revision == revision) + if (Revision != revision) { - // collection unchanged, use cached - return cached ?? UpdateCached(ref cached, state, factory); + // Clear cached value to force call to UpdateCached + cached = null; + // Collection changed, clear whole feature cache + flush = true; } - // collection changed, clear cache - Cache = default(TCache); - // empty cache is current revision - Revision = revision; - - return UpdateCached(ref cached, state, factory); + return cached ?? UpdateCached(ref cached, state, factory, revision, flush); } - private TFeature UpdateCached(ref TFeature cached, TState state, Func factory) where TFeature : class + // Update and cache clearing logic, when the fast-path in Fetch isn't applicable + private TFeature UpdateCached(ref TFeature cached, TState state, Func factory, int revision, bool flush) where TFeature : class { + if (flush) + { + // Collection detected as changed, clear cache + Cache = default(TCache); + } + cached = Collection.Get(); if (cached == null) { - // create if item not in collection + // Item not in collection, create it with factory cached = factory(state); + // Add item to IFeatureCollection Collection.Set(cached); - // Revision changed by .Set, update revision + // Revision changed by .Set, update revision to new value Revision = Collection.Revision; } + else if (flush) + { + // Cache was cleared, but item retrived from current Collection for version + // so use passed in revision rather than making another virtual call + Revision = revision; + } return cached; } diff --git a/src/Microsoft.AspNetCore.Http/Authentication/DefaultAuthenticationManager.cs b/src/Microsoft.AspNetCore.Http/Authentication/DefaultAuthenticationManager.cs index e6540d2b7d..666e2179e1 100644 --- a/src/Microsoft.AspNetCore.Http/Authentication/DefaultAuthenticationManager.cs +++ b/src/Microsoft.AspNetCore.Http/Authentication/DefaultAuthenticationManager.cs @@ -13,6 +13,9 @@ namespace Microsoft.AspNetCore.Http.Authentication.Internal { public class DefaultAuthenticationManager : AuthenticationManager { + // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _newAuthenticationFeature = f => new HttpAuthenticationFeature(); + private HttpContext _context; private FeatureReferences _features; @@ -35,7 +38,7 @@ namespace Microsoft.AspNetCore.Http.Authentication.Internal public override HttpContext HttpContext => _context; private IHttpAuthenticationFeature HttpAuthenticationFeature => - _features.Fetch(ref _features.Cache, f => new HttpAuthenticationFeature()); + _features.Fetch(ref _features.Cache, _newAuthenticationFeature); public override IEnumerable GetAuthenticationSchemes() { diff --git a/src/Microsoft.AspNetCore.Http/DefaultHttpContext.cs b/src/Microsoft.AspNetCore.Http/DefaultHttpContext.cs index 7feb5324a6..d1e431c7fa 100644 --- a/src/Microsoft.AspNetCore.Http/DefaultHttpContext.cs +++ b/src/Microsoft.AspNetCore.Http/DefaultHttpContext.cs @@ -15,6 +15,15 @@ namespace Microsoft.AspNetCore.Http { public class DefaultHttpContext : HttpContext { + // Lambdas hoisted to static readonly fields to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _newItemsFeature = f => new ItemsFeature(); + private readonly static Func _newServiceProvidersFeature = f => new ServiceProvidersFeature(); + private readonly static Func _newHttpAuthenticationFeature = f => new HttpAuthenticationFeature(); + private readonly static Func _newHttpRequestLifetimeFeature = f => new HttpRequestLifetimeFeature(); + private readonly static Func _newSessionFeature = f => new DefaultSessionFeature(); + private readonly static Func _nullSessionFeature = f => null; + private readonly static Func _newHttpRequestIdentifierFeature = f => new HttpRequestIdentifierFeature(); + private FeatureReferences _features; private HttpRequest _request; @@ -73,26 +82,26 @@ namespace Microsoft.AspNetCore.Http } private IItemsFeature ItemsFeature => - _features.Fetch(ref _features.Cache.Items, f => new ItemsFeature()); + _features.Fetch(ref _features.Cache.Items, _newItemsFeature); private IServiceProvidersFeature ServiceProvidersFeature => - _features.Fetch(ref _features.Cache.ServiceProviders, f => new ServiceProvidersFeature()); + _features.Fetch(ref _features.Cache.ServiceProviders, _newServiceProvidersFeature); private IHttpAuthenticationFeature HttpAuthenticationFeature => - _features.Fetch(ref _features.Cache.Authentication, f => new HttpAuthenticationFeature()); + _features.Fetch(ref _features.Cache.Authentication, _newHttpAuthenticationFeature); private IHttpRequestLifetimeFeature LifetimeFeature => - _features.Fetch(ref _features.Cache.Lifetime, f => new HttpRequestLifetimeFeature()); + _features.Fetch(ref _features.Cache.Lifetime, _newHttpRequestLifetimeFeature); private ISessionFeature SessionFeature => - _features.Fetch(ref _features.Cache.Session, f => new DefaultSessionFeature()); + _features.Fetch(ref _features.Cache.Session, _newSessionFeature); private ISessionFeature SessionFeatureOrNull => - _features.Fetch(ref _features.Cache.Session, f => null); + _features.Fetch(ref _features.Cache.Session, _nullSessionFeature); private IHttpRequestIdentifierFeature RequestIdentifierFeature => - _features.Fetch(ref _features.Cache.RequestIdentifier, f => new HttpRequestIdentifierFeature()); + _features.Fetch(ref _features.Cache.RequestIdentifier, _newHttpRequestIdentifierFeature); public override IFeatureCollection Features => _features.Collection; diff --git a/src/Microsoft.AspNetCore.Http/Features/QueryFeature.cs b/src/Microsoft.AspNetCore.Http/Features/QueryFeature.cs index 92d9aaa342..36781ef16e 100644 --- a/src/Microsoft.AspNetCore.Http/Features/QueryFeature.cs +++ b/src/Microsoft.AspNetCore.Http/Features/QueryFeature.cs @@ -9,6 +9,9 @@ namespace Microsoft.AspNetCore.Http.Features { public class QueryFeature : IQueryFeature { + // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _nullRequestFeature = f => null; + private FeatureReferences _features; private string _original; @@ -35,7 +38,7 @@ namespace Microsoft.AspNetCore.Http.Features } private IHttpRequestFeature HttpRequestFeature => - _features.Fetch(ref _features.Cache, f => null); + _features.Fetch(ref _features.Cache, _nullRequestFeature); public IQueryCollection Query { diff --git a/src/Microsoft.AspNetCore.Http/Features/RequestCookiesFeature.cs b/src/Microsoft.AspNetCore.Http/Features/RequestCookiesFeature.cs index 49ae550676..cd37b360a4 100644 --- a/src/Microsoft.AspNetCore.Http/Features/RequestCookiesFeature.cs +++ b/src/Microsoft.AspNetCore.Http/Features/RequestCookiesFeature.cs @@ -11,6 +11,9 @@ namespace Microsoft.AspNetCore.Http.Features { public class RequestCookiesFeature : IRequestCookiesFeature { + // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _nullRequestFeature = f => null; + private FeatureReferences _features; private StringValues _original; private IRequestCookieCollection _parsedValues; @@ -36,7 +39,7 @@ namespace Microsoft.AspNetCore.Http.Features } private IHttpRequestFeature HttpRequestFeature => - _features.Fetch(ref _features.Cache, f => null); + _features.Fetch(ref _features.Cache, _nullRequestFeature); public IRequestCookieCollection Cookies { diff --git a/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs b/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs index 59b8f68bb8..a62a02f4dc 100644 --- a/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs +++ b/src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs @@ -13,6 +13,9 @@ namespace Microsoft.AspNetCore.Http.Features /// public class ResponseCookiesFeature : IResponseCookiesFeature { + // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _nullResponseFeature = f => null; + // Object pool will be null only in test scenarios e.g. if code news up a DefaultHttpContext. private readonly ObjectPool _builderPool; @@ -50,7 +53,7 @@ namespace Microsoft.AspNetCore.Http.Features _builderPool = builderPool; } - private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, f => null); + private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, _nullResponseFeature); /// public IResponseCookies Cookies diff --git a/src/Microsoft.AspNetCore.Http/Internal/DefaultConnectionInfo.cs b/src/Microsoft.AspNetCore.Http/Internal/DefaultConnectionInfo.cs index 02db70767e..496df69654 100644 --- a/src/Microsoft.AspNetCore.Http/Internal/DefaultConnectionInfo.cs +++ b/src/Microsoft.AspNetCore.Http/Internal/DefaultConnectionInfo.cs @@ -1,6 +1,7 @@ // 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.Net; using System.Security.Cryptography.X509Certificates; using System.Threading; @@ -11,6 +12,10 @@ namespace Microsoft.AspNetCore.Http.Internal { public class DefaultConnectionInfo : ConnectionInfo { + // Lambdas hoisted to static readonly fields to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _newHttpConnectionFeature = f => new HttpConnectionFeature(); + private readonly static Func _newTlsConnectionFeature = f => new TlsConnectionFeature(); + private FeatureReferences _features; public DefaultConnectionInfo(IFeatureCollection features) @@ -29,10 +34,10 @@ namespace Microsoft.AspNetCore.Http.Internal } private IHttpConnectionFeature HttpConnectionFeature => - _features.Fetch(ref _features.Cache.Connection, f => new HttpConnectionFeature()); + _features.Fetch(ref _features.Cache.Connection, _newHttpConnectionFeature); private ITlsConnectionFeature TlsConnectionFeature=> - _features.Fetch(ref _features.Cache.TlsConnection, f => new TlsConnectionFeature()); + _features.Fetch(ref _features.Cache.TlsConnection, _newTlsConnectionFeature); public override IPAddress RemoteIpAddress { diff --git a/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpRequest.cs b/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpRequest.cs index f761cec1ae..c20164ad9c 100644 --- a/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpRequest.cs +++ b/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpRequest.cs @@ -12,6 +12,12 @@ namespace Microsoft.AspNetCore.Http.Internal { public class DefaultHttpRequest : HttpRequest { + // Lambdas hoisted to static readonly fields to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _nullRequestFeature = f => null; + private readonly static Func _newQueryFeature = f => new QueryFeature(f); + private readonly static Func _newFormFeature = r => new FormFeature(r); + private readonly static Func _newRequestCookiesFeature = f => new RequestCookiesFeature(f); + private HttpContext _context; private FeatureReferences _features; @@ -35,16 +41,16 @@ namespace Microsoft.AspNetCore.Http.Internal public override HttpContext HttpContext => _context; private IHttpRequestFeature HttpRequestFeature => - _features.Fetch(ref _features.Cache.Request, f => null); + _features.Fetch(ref _features.Cache.Request, _nullRequestFeature); private IQueryFeature QueryFeature => - _features.Fetch(ref _features.Cache.Query, f => new QueryFeature(f)); + _features.Fetch(ref _features.Cache.Query, _newQueryFeature); private IFormFeature FormFeature => - _features.Fetch(ref _features.Cache.Form, this, f => new FormFeature(f)); + _features.Fetch(ref _features.Cache.Form, this, _newFormFeature); private IRequestCookiesFeature RequestCookiesFeature => - _features.Fetch(ref _features.Cache.Cookies, f => new RequestCookiesFeature(f)); + _features.Fetch(ref _features.Cache.Cookies, _newRequestCookiesFeature); public override PathString PathBase { diff --git a/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpResponse.cs b/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpResponse.cs index 2073f8255e..9b664111cd 100644 --- a/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpResponse.cs +++ b/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpResponse.cs @@ -11,6 +11,10 @@ namespace Microsoft.AspNetCore.Http.Internal { public class DefaultHttpResponse : HttpResponse { + // Lambdas hoisted to static readonly fields to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _nullResponseFeature = f => null; + private readonly static Func _newResponseCookiesFeature = f => new ResponseCookiesFeature(f); + private HttpContext _context; private FeatureReferences _features; @@ -32,10 +36,10 @@ namespace Microsoft.AspNetCore.Http.Internal } private IHttpResponseFeature HttpResponseFeature => - _features.Fetch(ref _features.Cache.Response, f => null); + _features.Fetch(ref _features.Cache.Response, _nullResponseFeature); private IResponseCookiesFeature ResponseCookiesFeature => - _features.Fetch(ref _features.Cache.Cookies, f => new ResponseCookiesFeature(f)); + _features.Fetch(ref _features.Cache.Cookies, _newResponseCookiesFeature); public override HttpContext HttpContext { get { return _context; } } diff --git a/src/Microsoft.AspNetCore.Http/Internal/DefaultWebSocketManager.cs b/src/Microsoft.AspNetCore.Http/Internal/DefaultWebSocketManager.cs index fe5f859dc4..477282408d 100644 --- a/src/Microsoft.AspNetCore.Http/Internal/DefaultWebSocketManager.cs +++ b/src/Microsoft.AspNetCore.Http/Internal/DefaultWebSocketManager.cs @@ -12,6 +12,10 @@ namespace Microsoft.AspNetCore.Http.Internal { public class DefaultWebSocketManager : WebSocketManager { + // Lambdas hoisted to static readonly fields to improve inlining https://github.com/dotnet/roslyn/issues/13624 + private readonly static Func _nullRequestFeature = f => null; + private readonly static Func _nullWebSocketFeature = f => null; + private FeatureReferences _features; public DefaultWebSocketManager(IFeatureCollection features) @@ -30,10 +34,10 @@ namespace Microsoft.AspNetCore.Http.Internal } private IHttpRequestFeature HttpRequestFeature => - _features.Fetch(ref _features.Cache.Request, f => null); + _features.Fetch(ref _features.Cache.Request, _nullRequestFeature); private IHttpWebSocketFeature WebSocketFeature => - _features.Fetch(ref _features.Cache.WebSockets, f => null); + _features.Fetch(ref _features.Cache.WebSockets, _nullWebSocketFeature); public override bool IsWebSocketRequest {