HeaderUtilities enhancements by JunTaoLuo · Pull Request #738 · aspnet/HttpAbstractions

JunTaoLuo · 2016-11-22T19:03:31Z

JunTaoLuo · 2016-11-22T19:05:45Z

            return long.TryParse(value, NumberStyles.None, NumberFormatInfo.InvariantInfo, out result);
        }

+        internal static unsafe bool TryParseInt32(StringSegment value, out int result)


Not sure if we should use the same name as the older slower TryParseInt32/64 since there are a few behaviour differences. We parse only positive numbers for example, and we don't check for overflows. Also, it's hard to know without looking at the source that the two versions have different performance.

At least add doc comments for the behavior expectations.

Does the old one need to parse negatives? I'd be surprised. You can probably have the old one call the new one.

Add some overflow checks.

Why is this internal and TryParseInt64 public?

Even tough this is in the HeaderUtilites class, I think that it should probably be renamed. If not, the doc comments should clearly explain why it's different from int.TryParse.

Yea not sure why the old ParseInt32 is internal but I kept the same encapsulation. Should we make that public too?

only if it needs to be

Ok then, so TryParseInt64 is public because you're using it from Kestrel then? There should be doc comments on the public one explaining the limitations.

JunTaoLuo · 2016-11-22T19:07:40Z

+        private static bool TryParseInt64FromHeaderValue(int startIndex, string headerValue, out long value)
+        {
+            var found = false;
+            while (startIndex != headerValue.Length)


Could potentially turn this into unsafe code too. Not sure if that makes it any faster.

halter73 · 2016-11-22T19:34:42Z

-        private const string PrivateString = "private";
-        private const string ProxyRevalidateString = "proxy-revalidate";
-        private const string PublicString = "public";
-        private const string SharedMaxAgeString = "s-maxage";


Have we learned nothing from #712!? I actually prefer const as you can see if you read the PR comments, but we should be consistent and use static readonly instead.

Apparently we were using these values in a switch statement and they need to be const for that.

I don't know if David will be OK with this because of the switch statement. Like I said, I'm fine with consts given we don't mess up the values, but I would like consistency.

Modified the switch statement. These strings are now public static readonly.

👎 This regressed the code readability and maintainability. These constants aren't worth it.

Also avoiding a ToLower() which is required since switch on strings are case-sensitive. I'm seeing a one percent performance increase in ResponseCaching with it. Also it's code within a private method, so I'm not too concerned about its readability.

Tratcher · 2016-11-22T19:30:03Z

-        private const string ProxyRevalidateString = "proxy-revalidate";
-        private const string PublicString = "public";
-        private const string SharedMaxAgeString = "s-maxage";
+        public const string MaxAgeString = "max-age";


public static readonly string

Tratcher · 2016-11-22T19:32:15Z

            return long.TryParse(value, NumberStyles.None, NumberFormatInfo.InvariantInfo, out result);
        }

+        internal static unsafe bool TryParseInt32(StringSegment value, out int result)


At least add doc comments for the behavior expectations.

Tratcher · 2016-11-22T19:33:57Z

            return long.TryParse(value, NumberStyles.None, NumberFormatInfo.InvariantInfo, out result);
        }

+        internal static unsafe bool TryParseInt32(StringSegment value, out int result)


Does the old one need to parse negatives? I'd be surprised. You can probably have the old one call the new one.

Add some overflow checks.

Tratcher · 2016-11-22T19:36:25Z

+            }
+        }
+
+        public static unsafe bool TryParseInt64(StringSegment value, out long result)


I was testing it through TryParseTimeSpan but I'll add some tests specific to this method.

Tratcher · 2016-11-22T20:22:40Z

Consume the new API from Request/Response.ContentLength

JunTaoLuo · 2016-11-22T20:35:51Z

+                if (ch != end)
+                {
+                    result = 0;
+                    return true;


JunTaoLuo · 2016-11-22T20:36:02Z

+                if (ch != end)
+                {
+                    result = 0;
+                    return true;


JunTaoLuo · 2016-11-23T00:17:18Z

🆙📅

JunTaoLuo · 2016-11-24T03:07:32Z

🆙📅 @BrennanConroy @Tratcher @halter73 @davidfowl

Here's some performance results tested with ResponseCaching:
Before: 323984 RPS, 1379 bytes per request
After: 346946 RPS, 848 bytes per request

Tratcher · 2016-11-29T19:50:57Z

            if (rawValue.Count == 1 &&
                !string.IsNullOrWhiteSpace(rawValue[0]) &&
-                long.TryParse(rawValue[0], styles, CultureInfo.InvariantCulture, out value))
+                HeaderUtilities.TryParseInt64(new StringSegment(rawValue[0]).Trim(), out value))


We should do SetContentLength while we're at it, but I don't know if we have any optimized code for this yet. Kestrel has one but it's for hex.

Modified the FormatInt64 implementation.

As a reference, formatting long num = 1234567890; to "1234567890" for 30000000 iterations:
Old: 2027ms using num.ToString(CultureInfo.InvariantCulture)
New: 1023ms using FormatInt64(num)

/cc @KrzysztofCwalina

Tratcher · 2016-11-29T19:55:36Z

+        /// <code>false</code>.
+        /// </returns>
+        // e.g. { "headerValue=10, targetHeaderValue=30" }
+        public static bool TryParseTimeSpan(StringValues headerValues, string targetValue, out TimeSpan? value)


This name is misleading, you're not parsing a fully formatted time, only integer seconds. You just happen to represent the output as a TimeSpan. TryParseSeconds?

Tratcher · 2016-11-29T19:56:46Z

+        {
+            for (var i = 0; i < headerValues.Count; i++)
+            {
+                var index = headerValues[i].IndexOf(targetValue, StringComparison.OrdinalIgnoreCase);


This will give you false positives for partial matches

Not only partial matches, this also doesn't differentiate between name and value. i.e. header: foo=bar would be considered to contain bar where it should only contain foo. This needs a bit of rework.

JunTaoLuo · 2016-12-06T23:22:33Z

🆙📅

cc @Tratcher @halter73 @BrennanConroy

Tratcher · 2016-12-07T00:05:54Z

+            return false;
+        }
+
+        private static unsafe bool TryParseInt64FromHeaderValue(int startIndex, string headerValue, out long result)


Updated the implementation.

BrennanConroy · 2016-12-07T00:41:51Z

+            {
+                // Consider using Math.DivRem()
+                var quotient = value / 10;
+                charBuffer[--position] = (char)(0x30 + (value - quotient * 10)); // 0x30 = '0'


Use a char* here so we don't need to load position

I gave this a try but there isn't any improvement.

- Add allocation free parsing of int32, int64 - Improve performance of converting int64 to string - Add parsing of seconds from header values - Add check for existence of cache directives - Expose CacheControlHeaderValue constants

dnfclas added the cla-already-signed label Nov 22, 2016

JunTaoLuo commented Nov 22, 2016

View reviewed changes

This was referenced Nov 22, 2016

Header parsing aspnet/ResponseCaching#70

Merged

Move Int64 parsing logic to HttpAbstractions aspnet/KestrelHttpServer#1228

Merged

halter73 reviewed Nov 22, 2016

View reviewed changes

Tratcher reviewed Nov 22, 2016

View reviewed changes

JunTaoLuo commented Nov 22, 2016

View reviewed changes

Tratcher reviewed Nov 29, 2016

View reviewed changes

JunTaoLuo force-pushed the johluo/header-utilities branch from 270cf93 to bc7e27f Compare December 5, 2016 23:49

Tratcher reviewed Dec 7, 2016

View reviewed changes

BrennanConroy reviewed Dec 7, 2016

View reviewed changes

Tratcher approved these changes Dec 8, 2016

View reviewed changes

JunTaoLuo force-pushed the johluo/header-utilities branch from 9945d6d to ae2afb2 Compare December 9, 2016 01:21

Add functionalities to HeaderUtilities

d50a241

- Add allocation free parsing of int32, int64 - Improve performance of converting int64 to string - Add parsing of seconds from header values - Add check for existence of cache directives - Expose CacheControlHeaderValue constants

JunTaoLuo force-pushed the johluo/header-utilities branch from ae2afb2 to d50a241 Compare December 9, 2016 01:43

JunTaoLuo merged commit d50a241 into dev Dec 9, 2016

JunTaoLuo deleted the johluo/header-utilities branch December 9, 2016 22:17

Uh oh!

Conversation

JunTaoLuo commented Nov 22, 2016

Uh oh!

JunTaoLuo Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo commented Nov 23, 2016

Uh oh!

JunTaoLuo commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo commented Dec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JunTaoLuo Nov 22, 2016 •

edited

Loading

JunTaoLuo commented Nov 24, 2016 •

edited

Loading

JunTaoLuo Dec 6, 2016 •

edited

Loading