Skip to content
This repository has been archived by the owner. It is now read-only.

Reduce FeatureReference checking, lazy init FormFeature #999

Closed
wants to merge 4 commits into from

Conversation

@benaadams
Copy link
Contributor

commented Feb 10, 2018

Update to #925

Resolves #924 "FeatureCache is over-checked and over-reset"
Resolves #677 "FormFeature allocates when unused"
Resolves #880 "Don't allocate the FormFeature eagerly per request"

/cc @davidfowl

@@ -44,15 +46,26 @@ public DefaultHttpContext()
}

public DefaultHttpContext(IFeatureCollection features)
: this(new FeatureCollection(), FormOptions.Default)

This comment has been minimized.

Copy link
@davidfowl

davidfowl Feb 10, 2018

Member

Shouldn’t we be passing the feature collection and not a new one?

This comment has been minimized.

Copy link
@benaadams

benaadams Feb 10, 2018

Author Contributor

Done

@@ -158,7 +158,9 @@ public void UpdateFeatures_ClearsCachedFeatures()
features.Set<IHttpWebSocketFeature>(new TestHttpWebSocketFeature());

// featurecollection is set. all cached interfaces are null.
var context = new DefaultHttpContext(features);
var context = new DefaultHttpContext(features, FormOptions.Default);

This comment has been minimized.

Copy link
@davidfowl

davidfowl Feb 10, 2018

Member

Make sure we validate the default ctor as well

This comment has been minimized.

Copy link
@benaadams

benaadams Feb 10, 2018

Author Contributor

Done

@benaadams benaadams force-pushed the benaadams:feature-references branch from 9aff7c4 to d1649ea Feb 10, 2018
@benaadams

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2018

Before
image

After
image

Still a bucket load of calls; but less

@@ -8,6 +8,8 @@ namespace Microsoft.AspNetCore.Http.Features
{
public class FormOptions
{
public static readonly FormOptions Default = new FormOptions();

This comment has been minimized.

Copy link
@Tratcher

Tratcher Feb 12, 2018

Member

We can't make a mutable instance a public static. Make it internal?

This comment has been minimized.

Copy link
@benaadams

benaadams Feb 13, 2018

Author Contributor

Good point

benaadams added 2 commits Feb 13, 2018
fix
@@ -15,26 +15,39 @@ 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<(HttpRequest request, FormOptions options), IFormFeature> _newFormFeature = (r) => new FormFeature(r.request, r.options ?? FormOptions.Default);

This comment has been minimized.

Copy link
@davidfowl

davidfowl Feb 13, 2018

Member

Tuples? 😄

This comment has been minimized.

Copy link
@benaadams

benaadams Feb 14, 2018

Author Contributor

Custom struct or another Fetch overload?

@natemcmaster natemcmaster changed the base branch from dev to master Jul 2, 2018
@muratg

This comment has been minimized.

Copy link

commented Oct 10, 2018

@davidfowl @Tratcher is this still relevant/useful in your opinion? If so, let's move it forward. If not, let's get it closed.

Ben, thanks for the PR!

@muratg muratg added this to the 2.2.0-preview3 milestone Oct 10, 2018
@davidfowl

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

@benaadams I'm going to move this to 3.0

@davidfowl davidfowl modified the milestones: 2.2.0-preview3, 3.0.0 Nov 5, 2018
@natemcmaster

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

@davidfowl I'm about to merge this repo into aspnet/AspNetCore. If you'd like this PR merged, can you bring it in today? If not, I'm going to close this and you'll have to re-open the PR.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

@natemcmaster close it and I'll make @benaadams reopen it 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.