Less alloc/wrapping/boxing for Query, Forms, Cookies #411

benaadams · 2015-09-20T23:08:11Z

Lower allocation for empty dictionaries of types HeaderDictionary, QueryCollection, FormCollection, RequestCookies
No allocation for empty Form parsing or Query parsing
Read/Write request data for middleware editing/processing (e.g. 3rd party auth that removes its data; or data transforms along the pipeline)
Removed IReadableStringCollection, ReadableStringCollection -> IQueryCollection, IRequestCookies, IFormCollection each implementing IDictionary<string, StringValues>
Added IQueryCollection, IRequestCookies, IFormCollection doesn't throw on get contract, non-boxing enumerator in Empty paths.
KeyValueAccumulator from class to struct
Removed a bunch of boxing paths
Reduced allocs in some paths (e.g. ToUriComponent or ParseQuery)
Changes to support non-boxing Equality for StringValues (see PR Allow non-boxing equality for StringValues dotnet/extensions#55)

Also cleaned up and added some intellisense code comments; but they still need work, so this is not "done"

dnfclas · 2015-09-20T23:08:14Z

Hi @benaadams, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

benaadams · 2015-09-20T23:15:14Z

May need changes to Kestrel's custom IDictionaries dealing with empty keys
Also Content-Length bonding to header

Tratcher · 2015-09-21T01:33:21Z

src/Microsoft.AspNet.Http/Features/HttpRequestFeature.cs

@@ -12,7 +12,7 @@ public class HttpRequestFeature : IHttpRequestFeature
    {
        public HttpRequestFeature()


FYI: This implementation is used exclusively in unit tests, no optimizations required. We should add a doc comment to that effect.

Tratcher · 2015-09-21T15:22:57Z

@benaadams What you're getting at with much of this is that our multiple levels of abstraction are inefiicent and we should scale back. That's legit, but I think we need to choose which collection we use carefully. We used Dictionaries internally because they were convenient storage, but as fowler pointed out we hate that their index throws, and they also have a significantly larger surface area than we need.

Rather than pushing IDictionary up to the surface APIs, let's try pushing a reduced IHeaderCollection down into the features/servers. we only really need IEnumerable<KeyValuePair<string, StringValues>>, Count, indexer, and ICollection<Keys>. @davidfowl, @lodejard, what do you think?

Tratcher · 2015-09-21T16:07:28Z

The same goes for query, form, etc..

davidfowl · 2015-09-21T16:49:20Z

@Tratcher we can't push the form and query etc down to the server but that's not what you mean I assume.

Tratcher · 2015-09-21T17:00:03Z

@davidfowl Not down to the server, but down to their respective storage/features.

benaadams · 2015-09-21T17:03:41Z

@Tratcher yes. However, after a large amount of changes, and some iterations to maintain a close analogue to flow and constraints, I mostly ended up in a similar place to where the code started; including wrapping on get for headers!

There are a few tweaks but they really end up independent to the abstraction changes (LazyDict and its inherited collections, KeyValueAccumulator). Which was't where I expected to be; and would have likely been the exact same place if I wasn't trying to avoid it. 😿

However, I did ponder along the journey...

Questions

Is wrapping the collections for no "throw on get" necessary? I completely agree with the reasons; but could this be a documented constraint for writing a custom server or implementing IHttpRequestFeature / IHttpResponseFeature? For example Kestrel's FrameHeaders collection will not throw on get and that will probably be the default for the majority of people. So is this overly defensive?

Equally the default collections for forms and query are again provided so can be wrapped at that point (as per this PR with FormCollection : SafeLazyDictionary ignoring the inheritance exposure issue)
Is the request data being read-only desirable? With the request data being read-only this means middleware providers are more likely to resolve down to IHttpRequestFeature and wholesale replace the collection as they can't otherwise alter the data; making the defensive approach in the above question more necessary.

I had a few more, but I think they are artefacts of the above two and what implementation the answers encourage you down. Not necessarily looking for answers.

Tratcher · 2015-09-21T17:24:38Z

IDictionary's contract is that the indexer throws, that's not something we can/should change in our stack as it would case a lot of unpleasant surprises. If we want a different semantic then we need a different contract / interface. I don't think we even need IDictionary, it was just the closest match to what wanted. Now that we're cleaning things up I think we can come up with a slimmed down type that has the semantics we want and use that in both the surface API (HttpRequest) and internally (IHttpRequestFeature).

The request data should be mutable. There are common scenarios where middleware will alter the request to meet downstream requirements like applying x-forwarded headers to their respective fields.

benaadams · 2015-09-21T18:14:40Z

Different Contract makes sense; also with IgnoreCase on keys?

If you did something like this (inheritance-wise) everyone will want some:

interface IReadOnly<TKey, TValue>
{
    TValue Item { get; }
}

interface IReadWrite<TKey, TValue> : IReadOnly<TKey, TValue>
{
    new TValue Item { get; set; }
}

It seems to work, though you have to implement the get twice:

class Thing<TKey, TValue> : IReadWrite<TKey, TValue>
{
    public TValue Item
    {
        get
        {
            throw new NotImplementedException();
        }

        set
        {
            throw new NotImplementedException();
        }
    }

    TValue IReadOnly<TKey, TValue>.Item
    {
        get
        {
            return Item;
        }
    }
}

Then you can create a concrete type; use it read/write via interface and expose as readonly without any as casting between IReadWrite and IReadOnly only upcasting.

var thing = Thing();

thing["that"] = that;

IReadWrite iThing = thing;
iThing["other"] = other;

return (IReadOnly)iThing;

benaadams · 2015-10-30T23:42:30Z

Rebased rather than merged

dnfclas · 2015-10-31T01:43:12Z

Hi @benaadams, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

Tratcher · 2015-11-02T16:56:16Z

@rynowak How badly will this break MVC?

Tratcher · 2015-11-02T17:02:02Z

src/Microsoft.AspNet.Http.Extensions/HeaderDictionaryTypeExtensions.cs

+
+                switch (values.Count)
+                {
+                    case 0:


values.Count == 0 was already checked in the prior if block. Might as well make this if..else if...else.

rynowak · 2015-11-02T17:07:19Z

@Tratcher - I'd expect much more breakage in the tests which frequently new up collections for testing. We actually read these very few places in the product code.

Tratcher · 2015-11-02T20:11:56Z

src/Microsoft.AspNet.Http/RequestCookieCollection.cs

+            }
+        }
+
+        StringValues IRequestCookieCollection.this[string key]


Something is strange here, there shouldn't be two indexers.

string and StringValues

Ah, right, this collection really is different from the others. It doesn't need to use StringValues anywhere, it only had that because of the old common IReadableStringCollection. We can just make this all <string, string> now.

Ah ok, makes things simpiler :)

Hmm... how does that fit with RequestCookieFeature? https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.Http/Features/RequestCookiesFeature.cs#L62

Multiple entries for the same value as via headers? Concat?

nvm; I think I've got it

Tratcher · 2015-11-02T20:31:15Z

This is pretty much done. I'll rebase and merge it when I get a chance later today. Any of the minor remaining comments you don't get to by then I'll just take care of. Thanks

benaadams · 2015-11-02T21:03:56Z

Done, however quick check am returning string.Empty where a cookie doesn't exist which is "" rather than null; is this the expected behavior?

Tratcher · 2015-11-02T21:13:52Z

Should be fine. If they really care they can use TryGetValue

Tratcher · 2015-11-02T23:19:12Z

Rebased, squashed, merged.

benaadams · 2015-11-03T02:24:47Z

👏 :)

NickCraver · 2015-11-03T03:25:46Z

src/Microsoft.AspNet.Http.Extensions/UriHelper.cs

@@ -25,7 +25,7 @@ public static class UriHelper
            FragmentString fragment = new FragmentString())
        {
            string combinePath = (pathBase.HasValue || path.HasValue) ? (pathBase + path).ToString() : "/";
-            return combinePath + query + fragment;
+            return $"{combinePath}{query.ToString()}{fragment.ToString()}";


Since this has just 3 components, it would be more efficient reverting to the String.Concat() a+b+c format it was in before. While maintaining equal readability, it's ~3-4x faster on most machines (I've tested this on a variety locally and on the latest servers).

Send a PR 😄

This adds additional performance improvements (namely string.Concat overloads) on top of aspnet#411.

dnfclas added the cla-already-signed label Sep 20, 2015

Tratcher reviewed Sep 21, 2015
View reviewed changes

benaadams changed the title ~~[Design] Headers, Query, Forms, Cookies improvements~~ [Discuss] Headers, Query, Forms, Cookies improvements Sep 21, 2015

Move extension to existing Extenstions class

a0caefd

benaadams closed this Oct 31, 2015

benaadams reopened this Oct 31, 2015

dnfclas added the cla-already-signed label Oct 31, 2015

Tratcher added this to the 1.0.0-rc2 milestone Nov 2, 2015

Tratcher self-assigned this Nov 2, 2015

Tratcher reviewed Nov 2, 2015
View reviewed changes

cr feedback

90b8f4f

Tratcher reviewed Nov 2, 2015
View reviewed changes

Simplify Cookies Interface

003152d

Tratcher closed this Nov 2, 2015

NickCraver reviewed Nov 3, 2015
View reviewed changes

NickCraver added a commit to NickCraver/HttpAbstractions that referenced this pull request Nov 3, 2015

Performance improvements

bacf760

This adds additional performance improvements (namely string.Concat overloads) on top of aspnet#411.

benaadams deleted the lazy-dict branch May 10, 2016 11:28

benaadams mentioned this pull request May 26, 2016

KeyValueAccumulator performance improvements #368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Less alloc/wrapping/boxing for Query, Forms, Cookies #411

Less alloc/wrapping/boxing for Query, Forms, Cookies #411

benaadams commented Sep 20, 2015

dnfclas commented Sep 20, 2015

benaadams commented Sep 20, 2015

Tratcher Sep 21, 2015

Tratcher commented Sep 21, 2015

Tratcher commented Sep 21, 2015

davidfowl commented Sep 21, 2015

Tratcher commented Sep 21, 2015

benaadams commented Sep 21, 2015

Tratcher commented Sep 21, 2015

benaadams commented Sep 21, 2015

benaadams commented Oct 30, 2015

dnfclas commented Oct 31, 2015

Tratcher commented Nov 2, 2015

Tratcher Nov 2, 2015

rynowak commented Nov 2, 2015

Tratcher Nov 2, 2015

benaadams Nov 2, 2015

Tratcher Nov 2, 2015

benaadams Nov 2, 2015

benaadams Nov 2, 2015

benaadams Nov 2, 2015

Tratcher commented Nov 2, 2015

benaadams commented Nov 2, 2015

Tratcher commented Nov 2, 2015

Tratcher commented Nov 2, 2015

benaadams commented Nov 3, 2015

NickCraver Nov 3, 2015

davidfowl Nov 3, 2015

		@@ -12,7 +12,7 @@ public class HttpRequestFeature : IHttpRequestFeature
		{
		public HttpRequestFeature()

Less alloc/wrapping/boxing for Query, Forms, Cookies #411

Less alloc/wrapping/boxing for Query, Forms, Cookies #411

Conversation

benaadams commented Sep 20, 2015

dnfclas commented Sep 20, 2015

benaadams commented Sep 20, 2015

Choose a reason for hiding this comment

Tratcher commented Sep 21, 2015

Tratcher commented Sep 21, 2015

davidfowl commented Sep 21, 2015

Tratcher commented Sep 21, 2015

benaadams commented Sep 21, 2015

Tratcher commented Sep 21, 2015

benaadams commented Sep 21, 2015

benaadams commented Oct 30, 2015

dnfclas commented Oct 31, 2015

Tratcher commented Nov 2, 2015

Choose a reason for hiding this comment

rynowak commented Nov 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Nov 2, 2015

benaadams commented Nov 2, 2015

Tratcher commented Nov 2, 2015

Tratcher commented Nov 2, 2015

benaadams commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment