[Perf] HttpContextFactory should only wrap ReadOnly feature collections

[Tratcher]

https://github.com/aspnet/Hosting/blob/dev/src/Microsoft.AspNet.Hosting/Builder/HttpContextFactory.cs#L14

IFeatureCollections can be marked as IsReadOnly or not. HttpContextFactory currently assumes they are all read only and wraps them in a mutable FeatureCollection. This has some perf overhead where it creates extra Dictionaries, etc, and is redundant if the server already provided a mutable collection.

Recommendation: Only wrap the given feature collection if is marked as IsReadOnly.

[Tratcher]

Nevermind, FeatureCollection has made significant improvements in this scenario.

[Tratcher]

@lodejard @benaadams

[benaadams]

I did have a look at FeatureCollection when checking the codebase and in isolation its pretty efficient at what it does; so couldn't come up with any improvements.

Taking a step back there are FeatureCollection improvements coming in Kestrel; which regardless of metrics, have the distinct allocation advantage of already being created, and this wrapping would prevent it being utilised passed the point the context is created.

However, I don't have the holistic view on what circumstances; if any, IFeatureCollection could be readonly, and whether FeatureCollection is needed at all; but if it is, you could make the one line change from:

return new DefaultHttpContext(new FeatureCollection(featureCollection));

to what @Tratcher originally suggested (in preferred line break formatting)

return new DefaultHttpContext(
    featureCollection.IsReadOnly 
    ? new FeatureCollection(featureCollection) 
    : featureCollection);

which would work and allow the changes in Kestrel's collection to flow through. So that would be an easy fix?

Then open question on when would the IFeatureCollection be readonly? Bearing in mind throwing NotImplementedExecption at runtime is a pretty terrible thing.

[Tratcher]

Allowing the IFeatureCollection to be read-only can make for simpler server implementations. FeatureCollection makes up the difference.

Or we cut that and just ask the simple servers to use FeatureCollection directly.

[benaadams]

FeatureCollection is a very interesting collection; especially when you consider you can pass an FeatureCollection to its constructor; boom you've got javascript's prototype inheritance right there; change the original and it flows through if its not overridden at some stage. I digress...

Allowing the IFeatureCollection to be read-only can make for simpler server implementations.

Are there any existing implementations? Doing an if check/ternary operator here is about as lightweight as you can get, so if you want to support that scenario I don't think there is any disadvantage.

If there aren't any existing implementations, pushing out the readonly might make sense as its sort of a hidden intent coercion (e.g. "I made this readonly; with NotImplementedExecptions why can you change it and get no errors?")

So is definitely a design call.

Though I'd favour changing it to wrap conditionally, this instant as you've suggested; as it would be more performant and otherwise will cause issues with Kestrel's new implementation - then pick up readonly in an internal discussion? (even if its only 5 secs 😝)

However... as I said at the start; the FeatureCollection is pretty lightweight and has very interesting properties, and might work well for routing? Particularly aspnet/Routing#211: Optimize allocations in RouteData + Optimize the design of attribute routing where you could have default route paths per segment in a tree, that then are overridden and overridden again by attributes on the specifics; yet maintain their default routing where not overridden?

[benaadams]

In fact just made a PR for it...

[davidfowl]

I think we should keep IFeatureCollection and do the optimization.

[benaadams]

@davidfowl wasn't suggesting removing IFeatureCollection as a possibility, just ReadOnly. Though the optimization gives you 99.999% of the benefits; so probably isn't worth interface churn.

[Tratcher]

I was suggesting that Hosting doesn't need to wrap at all since it's trivial for servers to generate a mutable feature collection. See
https://github.com/aspnet/KestrelHttpServer/blob/49451fb11e53a1783a3b3415f228411de5ef45b0/src/Microsoft.AspNet.Server.Kestrel/ServerRequest.cs#L25
https://github.com/aspnet/KestrelHttpServer/blob/49451fb11e53a1783a3b3415f228411de5ef45b0/src/Microsoft.AspNet.Server.Kestrel/ServerRequest.cs#L208-L213

[davidfowl]

Yes, we can just make that part of the contract.