Make FeatureReferences<T>.Fetch inlineable (#704)

Author: benaadams

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 @@ public FeatureReferences(IFeatureCollection collection)
  // 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<TFeature, TState>(
  ref TFeature cached,
  TState state,
  Func<TState, TFeature> 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<TFeature, TState>(ref TFeature cached, TState state, Func<TState, TFeature> factory) where TFeature : class
+ // Update and cache clearing logic, when the fast-path in Fetch isn't applicable
+ private TFeature UpdateCached<TFeature, TState>(ref TFeature cached, TState state, Func<TState, TFeature> factory, int revision, bool flush) where TFeature : class
  {
+ if (flush)
+ {
+ // Collection detected as changed, clear cache
+ Cache = default(TCache);
+ }
+
  cached = Collection.Get<TFeature>();
  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;
  }

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<IFeatureCollection, IHttpAuthenticationFeature> _newAuthenticationFeature = f => new HttpAuthenticationFeature();
+
  private HttpContext _context;
  private FeatureReferences<IHttpAuthenticationFeature> _features;
 
@@ -35,7 +38,7 @@ public virtual void Uninitialize()
  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<AuthenticationDescription> GetAuthenticationSchemes()
  {

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<IFeatureCollection, IItemsFeature> _newItemsFeature = f => new ItemsFeature();
+ private readonly static Func<IFeatureCollection, IServiceProvidersFeature> _newServiceProvidersFeature = f => new ServiceProvidersFeature();
+ private readonly static Func<IFeatureCollection, IHttpAuthenticationFeature> _newHttpAuthenticationFeature = f => new HttpAuthenticationFeature();
+ private readonly static Func<IFeatureCollection, IHttpRequestLifetimeFeature> _newHttpRequestLifetimeFeature = f => new HttpRequestLifetimeFeature();
+ private readonly static Func<IFeatureCollection, ISessionFeature> _newSessionFeature = f => new DefaultSessionFeature();
+ private readonly static Func<IFeatureCollection, ISessionFeature> _nullSessionFeature = f => null;
+ private readonly static Func<IFeatureCollection, IHttpRequestIdentifierFeature> _newHttpRequestIdentifierFeature = f => new HttpRequestIdentifierFeature();
+
  private FeatureReferences<FeatureInterfaces> _features;
 
  private HttpRequest _request;
@@ -73,26 +82,26 @@ public virtual void Uninitialize()
  }
 
  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;
 

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<IFeatureCollection, IHttpRequestFeature> _nullRequestFeature = f => null;
+
  private FeatureReferences<IHttpRequestFeature> _features;
 
  private string _original;
@@ -35,7 +38,7 @@ public QueryFeature(IFeatureCollection features)
  }
 
  private IHttpRequestFeature HttpRequestFeature =>
- _features.Fetch(ref _features.Cache, f => null);
+ _features.Fetch(ref _features.Cache, _nullRequestFeature);
 
  public IQueryCollection Query
  {

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<IFeatureCollection, IHttpRequestFeature> _nullRequestFeature = f => null;
+
  private FeatureReferences<IHttpRequestFeature> _features;
  private StringValues _original;
  private IRequestCookieCollection _parsedValues;
@@ -36,7 +39,7 @@ public RequestCookiesFeature(IFeatureCollection features)
  }
 
  private IHttpRequestFeature HttpRequestFeature =>
- _features.Fetch(ref _features.Cache, f => null);
+ _features.Fetch(ref _features.Cache, _nullRequestFeature);
 
  public IRequestCookieCollection Cookies
  {

src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs

@@ -13,6 +13,9 @@ namespace Microsoft.AspNetCore.Http.Features
  /// </summary>
  public class ResponseCookiesFeature : IResponseCookiesFeature
  {
+ // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624
+ private readonly static Func<IFeatureCollection, IHttpResponseFeature> _nullResponseFeature = f => null;
+
  // Object pool will be null only in test scenarios e.g. if code news up a DefaultHttpContext.
  private readonly ObjectPool<StringBuilder> _builderPool;
 
@@ -50,7 +53,7 @@ public ResponseCookiesFeature(IFeatureCollection features, ObjectPool<StringBuil
  _builderPool = builderPool;
  }
 
- private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, f => null);
+ private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, _nullResponseFeature);
 
  /// <inheritdoc />
  public IResponseCookies Cookies

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<IFeatureCollection, IHttpConnectionFeature> _newHttpConnectionFeature = f => new HttpConnectionFeature();
+ private readonly static Func<IFeatureCollection, ITlsConnectionFeature> _newTlsConnectionFeature = f => new TlsConnectionFeature();
+
  private FeatureReferences<FeatureInterfaces> _features;
 
  public DefaultConnectionInfo(IFeatureCollection features)
@@ -29,10 +34,10 @@ public virtual void Uninitialize()
  }
 
  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
  {

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<IFeatureCollection, IHttpRequestFeature> _nullRequestFeature = f => null;
+ private readonly static Func<IFeatureCollection, IQueryFeature> _newQueryFeature = f => new QueryFeature(f);
+ private readonly static Func<HttpRequest, IFormFeature> _newFormFeature = r => new FormFeature(r);
+ private readonly static Func<IFeatureCollection, IRequestCookiesFeature> _newRequestCookiesFeature = f => new RequestCookiesFeature(f);
+
  private HttpContext _context;
  private FeatureReferences<FeatureInterfaces> _features;
 
@@ -35,16 +41,16 @@ public virtual void Uninitialize()
  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
  {

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<IFeatureCollection, IHttpResponseFeature> _nullResponseFeature = f => null;
+ private readonly static Func<IFeatureCollection, IResponseCookiesFeature> _newResponseCookiesFeature = f => new ResponseCookiesFeature(f);
+
  private HttpContext _context;
  private FeatureReferences<FeatureInterfaces> _features;
 
@@ -32,10 +36,10 @@ public virtual void Uninitialize()
  }
 
  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; } }

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<IFeatureCollection, IHttpRequestFeature> _nullRequestFeature = f => null;
+ private readonly static Func<IFeatureCollection, IHttpWebSocketFeature> _nullWebSocketFeature = f => null;
+
  private FeatureReferences<FeatureInterfaces> _features;
 
  public DefaultWebSocketManager(IFeatureCollection features)
@@ -30,10 +34,10 @@ public virtual void Uninitialize()
  }
 
  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
  {