Header parsing #70

BrennanConroy · 2016-10-07T17:34:31Z

Still a bit of cleanup needed.

Saved about 791 bytes per request.
1627.6 originally
836.6 after

Benchmarks has shown an average of 3.386% increase in RPS

Tratcher · 2016-10-07T17:41:00Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCacheContext.cs

-                    _requestHeaders = HttpContext.Request.GetTypedHeaders();
+                    _parsedResponseDate = true;
+                    DateTimeOffset date;
+                    if (DateTimeOffset.TryParse(HttpContext.Response.Headers[HeaderNames.Date], out date))


format/culture?

BrennanConroy · 2016-10-07T20:34:11Z

src/Microsoft.AspNetCore.ResponseCaching/CacheControlValues.cs

@@ -0,0 +1,23 @@
+using System;


BrennanConroy · 2016-10-07T20:34:23Z

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

@@ -0,0 +1,74 @@
+using System;


JunTaoLuo · 2016-10-07T23:22:51Z

src/Microsoft.AspNetCore.ResponseCaching/CacheControlValues.cs

+{
+    internal class CacheControlValues
+    {
+        public const string MaxAgeString = "max-age";


Any chance we make these strings public in HttpAbstractions so we don't duplicated it here?

JunTaoLuo · 2016-10-11T18:38:12Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCacheContext.cs

-                    _responseDate = TypedResponseHeaders.Date;
+                    _parsedResponseSharedMaxAge = true;
+                    _responseSharedMaxAge = null;
+                    foreach (var header in HttpContext.Response.Headers[HeaderNames.CacheControl])


Weren't we trying to eliminate the use of foreach to reduce allocations for enumerators?

JunTaoLuo · 2016-10-11T18:41:29Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCacheContext.cs

-                    _responseCacheControl = TypedResponseHeaders.CacheControl ?? EmptyCacheControl;
+                    _parsedResponseExpires = true;
+                    DateTimeOffset expires;
+                    if (ParsingHelpers.TryStringToDate(HttpContext.Response.Headers[HeaderNames.Expires], out expires))


TryStringToDate => TryParseDate

JunTaoLuo · 2016-10-11T18:42:44Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCacheContext.cs

-                    _responseExpires = TypedResponseHeaders.Expires;
+                    _parsedResponseMaxAge = true;
+                    _responseMaxAge = null;
+                    foreach (var header in HttpContext.Response.Headers[HeaderNames.CacheControl])


Code looks very similar to parsing ResponseSharedMaxAge, extract into a helper method in ParsingHelpers?

JunTaoLuo · 2016-10-11T19:16:54Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCachePolicyProvider.cs

-                    var expires = context.CachedResponseHeaders.Expires;
-                    if (responseTime >= expires)
+                    DateTimeOffset expires;
+                    if (context.CachedResponseHeaders[HeaderNames.Expires].Count > 0)


Combine the nested if statements.

JunTaoLuo · 2016-10-11T19:18:00Z

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

+        {
+            // Try the various date formats in the order listed above.
+            // We should accept a wide verity of common formats, but only output RFC 1123 style dates.
+            if (DateTimeOffset.TryParseExact(input, DateFormats, DateTimeFormatInfo.InvariantInfo,


so return DateTimeOffset.TryParseExact(...)? Why the if statement? In fact you can do => DateTimeOffset.TryParseExact(...)

JunTaoLuo · 2016-10-11T19:24:15Z

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

+            return false;
+        }
+
+        internal static bool TryParseHeaderValue(int startIndex, string header, out int value)


Add tests for this.

JunTaoLuo · 2016-10-11T19:25:18Z

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

+                while (endIndex < header.Length)
+                {
+                    var c = header[endIndex];
+                    if ((c >= '0') && (c <= '9'))


what about spaces between equals and the first number? max-age = 60?

I could have swore I tested that... will fix

JunTaoLuo · 2016-10-11T19:32:43Z

src/Microsoft.AspNetCore.ResponseCaching/ResponseCacheMiddleware.cs

-                            context.Logger.LogNotModifiedIfNoneMatchMatched(tag);
-                            return true;
+                            EntityTagHeaderValue requestETag;
+                            if (EntityTagHeaderValue.TryParse(tag, out requestETag))


combine if statements

JunTaoLuo · 2016-10-11T19:32:57Z

src/Microsoft.AspNetCore.ResponseCaching/ResponseCacheMiddleware.cs

-                    var lastModified = cachedResponseHeaders.LastModified ?? cachedResponseHeaders.Date;
-                    if (lastModified <= ifUnmodifiedSince)
+                    DateTimeOffset modified;
+                    if (!ParsingHelpers.TryStringToDate(cachedResponseHeaders[HeaderNames.LastModified], out modified))


Combine if statements

JunTaoLuo · 2016-10-11T21:01:06Z

We should probably see whether we want to do the parsing on demand each time or parse all of the cache-control values in one go, need to test the performance of the two approaches.

JunTaoLuo · 2016-10-12T01:12:07Z

test/Microsoft.AspNetCore.ResponseCaching.Tests/ParsingHelpersTests.cs

@@ -0,0 +1,35 @@
+using Microsoft.AspNetCore.ResponseCaching.Internal;


No copyright = copyleft and we don't like that

JunTaoLuo · 2016-10-12T01:14:58Z

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

@@ -0,0 +1,100 @@
+// Copyright (c) .NET Foundation. All rights reserved.


This file should go in the Internal folder.

JunTaoLuo · 2016-10-12T01:18:35Z

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

+        internal static bool TryParseDate(string input, out DateTimeOffset result) => DateTimeOffset.TryParseExact(input, DateFormats, DateTimeFormatInfo.InvariantInfo,
+                DateTimeStyles.AllowWhiteSpaces | DateTimeStyles.AssumeUniversal, out result);
+
+        internal static bool TryGetHeaderValue(StringValues headers, string headerName, out int value)


Hmm the method name isn't really descriptive. Maybe TryParseHeaderInt? Should also consolidate the naming so maybe the previous method should be renamed to TryParseHeaderDate

JunTaoLuo · 2016-10-12T01:19:52Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCachePolicyProvider.cs

            {
                context.Logger.LogResponseWithoutPublicNotCacheable();
                return false;
            }

            // Check no-store
-            if (context.RequestCacheControlHeaderValue.NoStore || context.ResponseCacheControlHeaderValue.NoStore)
+            foreach (var header in context.HttpContext.Request.Headers[HeaderNames.CacheControl])


This could be extracted in a helper as well. ParsingHelper.HeaderContains(string)

JunTaoLuo · 2016-10-12T01:24:30Z

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

+
+namespace Microsoft.AspNetCore.ResponseCaching.Internal
+{
+    internal static class ParsingHelpers


HttpHeaderParsingHelpers?

JunTaoLuo · 2016-10-12T01:26:01Z

src/Microsoft.AspNetCore.ResponseCaching/ResponseCacheMiddleware.cs

+                    DateTimeOffset unmodifiedSince;
+                    if (ParsingHelpers.TryParseDate(ifUnmodifiedSince, out unmodifiedSince))
+                    {
+                        if (modified <= unmodifiedSince)


JunTaoLuo · 2016-10-12T01:26:55Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCachePolicyProvider.cs

                    {
-                        context.Logger.LogExpirationExpiresExceeded(responseTime, expires.Value);
-                        return false;
+                        if (context.ResponseTime.Value >= expires)


JunTaoLuo · 2016-10-12T01:29:27Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/ResponseCachePolicyProvider.cs

+                    }
+
+                    TimeSpan? requestMaxStale = null;
+                    if (ParsingHelpers.TryGetHeaderValue(requestCacheControlHeaders, CacheControlValues.MaxStaleString, out seconds))


Actually this can be simplified by having a ParsingHelpers.TryParseTimeSpan (or some consistent naming) that returns a TimeSpan.

Can't pass a nullable as an out parameter :(

yes you can

pakrym · 2016-11-21T19:30:15Z

src/Microsoft.AspNetCore.ResponseCaching/Internal/HttpHeaderParsingHelpers.cs

+{
+    internal static class HttpHeaderParsingHelpers
+    {
+        private static readonly string[] DateFormats = new string[] {


Is this duplicated from HttpAbstractions?

It is private in that repo. We are considering moving a few parts of this pr to abstractions so we'll see if this is still needed here.

JunTaoLuo

I'm also taking a look at what we can move to Abstractions to eliminate duplication. Also, please review the changes in my latest commit.

JunTaoLuo · 2016-11-22T03:11:49Z

src/Microsoft.AspNetCore.ResponseCaching/ResponseCachingMiddleware.cs

                    {
-                        if (cachedResponseHeaders.ETag.Compare(tag, useStrongComparison: false))
+                        foreach (var tag in ifNoneMatchHeader)


I don't think you can use a foreach here since it's not guaranteed that each string contains only one ETag due how StringValues from HTTP headers are stored. Let's take a look at how it is parsed in Abstractions.

JunTaoLuo · 2016-11-22T03:13:01Z

src/Microsoft.AspNetCore.ResponseCaching/ResponseCachingMiddleware.cs

-                    var lastModified = cachedResponseHeaders.LastModified ?? cachedResponseHeaders.Date;
-                    if (lastModified <= ifUnmodifiedSince)
+                    DateTimeOffset modified;
+                    if (!HttpHeaderParsingHelpers.TryParseHeaderDate(cachedResponseHeaders[HeaderNames.LastModified], out modified) &&


I have this fixed in my clean up commit but just to note, both sides of the && operator are evaluated regardless of whether they are true or false. See example here: https://gist.github.com/JunTaoLuo/87f8b2e727c956333fa036f01b3275af

Um, the first one should be TrueStatement to actually test this

Oops, you're right. And in that case it works as you mentioned. Undid my changes. Must be confusing && with another operator we tested before.

JunTaoLuo · 2016-11-22T03:37:04Z

src/Microsoft.AspNetCore.ResponseCaching/ResponseCachingMiddleware.cs

@@ -365,20 +365,18 @@ internal static bool ContentIsNotModified(ResponseCachingContext context)
                    return true;
                }

-                if (!StringValues.IsNullOrEmpty(cachedResponseHeaders[HeaderNames.ETag]))
+                EntityTagHeaderValue eTag;
+                if (!StringValues.IsNullOrEmpty(cachedResponseHeaders[HeaderNames.ETag])


The previous comment got collapsed. But this need to be updated to match the parsing behaviour in Abstractions.

JunTaoLuo · 2016-11-22T19:08:47Z

Requires aspnet/HttpAbstractions#738

JunTaoLuo · 2016-12-09T23:36:40Z

Perf results on ResponseCachingPlaintextCached:
Before: 322984.7 RPS, 1657.2 bytes/req overhead from the responsecaching middleware
After: 346747.2 RPS, 1205.3 bytes/req overhead from the responsecaching middleware

BrennanConroy · 2016-12-09T23:48:20Z

🐑 🇮🇹

dnfclas added the cla-already-signed label Oct 7, 2016

Tratcher reviewed Oct 7, 2016

View reviewed changes

BrennanConroy commented Oct 7, 2016

View reviewed changes

src/Microsoft.AspNetCore.ResponseCaching/CacheControlValues.cs

@@ -0,0 +1,23 @@

using System;

Copy link

Member Author

BrennanConroy Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright

BrennanConroy commented Oct 7, 2016

View reviewed changes

src/Microsoft.AspNetCore.ResponseCaching/ParsingHelpers.cs

@@ -0,0 +1,74 @@

using System;

Copy link

Member Author

BrennanConroy Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright

JunTaoLuo reviewed Oct 7, 2016

View reviewed changes

JunTaoLuo suggested changes Oct 11, 2016

View reviewed changes

JunTaoLuo reviewed Oct 12, 2016

View reviewed changes

JunTaoLuo force-pushed the brecon/headers branch from e8de495 to 54be93e Compare November 1, 2016 20:55

JunTaoLuo force-pushed the brecon/headers branch from 54be93e to aba398d Compare November 18, 2016 01:23

pakrym reviewed Nov 21, 2016

View reviewed changes

JunTaoLuo suggested changes Nov 22, 2016

View reviewed changes

JunTaoLuo reviewed Nov 22, 2016

View reviewed changes

JunTaoLuo mentioned this pull request Nov 22, 2016

HeaderUtilities enhancements aspnet/HttpAbstractions#737

Closed

JunTaoLuo force-pushed the brecon/headers branch from 7322d8f to 3163c85 Compare November 23, 2016 00:41

BrennanConroy assigned JunTaoLuo Dec 8, 2016

Improve header parsing performance

9c94a77

JunTaoLuo force-pushed the brecon/headers branch from b8b6c9e to 313e047 Compare December 9, 2016 22:24

JunTaoLuo approved these changes Dec 9, 2016

View reviewed changes

JunTaoLuo force-pushed the brecon/headers branch from 313e047 to c9685cc Compare December 9, 2016 22:56

Relocate improvements to HttpAbstractions

e01431f

JunTaoLuo force-pushed the brecon/headers branch from c9685cc to e01431f Compare December 9, 2016 23:53

JunTaoLuo merged commit e01431f into dev Dec 9, 2016

JunTaoLuo deleted the brecon/headers branch December 9, 2016 23:53

JunTaoLuo mentioned this pull request Dec 12, 2016

Optimizations #87

Merged

		@@ -0,0 +1,35 @@
		using Microsoft.AspNetCore.ResponseCaching.Internal;

		@@ -0,0 +1,100 @@
		// Copyright (c) .NET Foundation. All rights reserved.

Header parsing #70

Header parsing #70

Conversation

BrennanConroy commented Oct 7, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

JunTaoLuo Oct 11, 2016 • edited Loading

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

JunTaoLuo commented Oct 11, 2016

JunTaoLuo Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo Oct 12, 2016 • edited Loading

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

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

JunTaoLuo left a comment • edited Loading

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

JunTaoLuo Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented Nov 22, 2016

JunTaoLuo commented Dec 9, 2016 • edited Loading

BrennanConroy commented Dec 9, 2016

JunTaoLuo Oct 11, 2016 •

edited

Loading

JunTaoLuo Oct 11, 2016 •

edited

Loading

JunTaoLuo Oct 12, 2016 •

edited

Loading

JunTaoLuo Oct 12, 2016 •

edited

Loading

JunTaoLuo left a comment •

edited

Loading

JunTaoLuo Nov 22, 2016 •

edited

Loading

JunTaoLuo commented Dec 9, 2016 •

edited

Loading