Use enum for method rather than string compares #2294

benaadams · 2018-02-03T23:12:44Z

For GET method IsHead test is performed on the string Method which since GET isn't reference equal to HEAD drops through to String.Equals -> OrdinalIgnoreCaseComparer.Equals

However the Parser starts with it as an Enum, which then gets converted to a string; so it can just be tested with the enum.

Rebased #2027

Resolves: #2020

davidfowl · 2018-02-03T23:24:54Z

src/Kestrel.Core/Internal/Http/Http1Connection.cs

            _httpVersion = version;

            Debug.Assert(RawTarget != null, "RawTarget was not set");
-            Debug.Assert(Method != null, "Method was not set");
+            Debug.Assert(((IHttpRequestFeature)this).Method != null, "Method was not set");


Why this change?

Ah, because method is an enum.

To get the string version

benaadams · 2018-02-03T23:35:42Z

OSX

Failed   RegisterAddresses_IPv6_Success(addressInput: "http://*:0/", testUrls: ["http://127.0.0.1", "http://[::1]"])
Error Message:
 System.Net.Sockets.SocketException : Operation timed out
Stack Trace:
   at Microsoft.AspNetCore.Testing.HttpClientSlim.GetSocket(Uri requestUri)
   at Microsoft.AspNetCore.Testing.HttpClientSlim.GetStream(Uri requestUri, Boolean validateCertificate)
   at Microsoft.AspNetCore.Testing.HttpClientSlim.GetStringAsync(Uri requestUri, Boolean validateCertificate)
   at Microsoft.AspNetCore.Testing.HttpClientSlim.GetStringAsync(String requestUri, Boolean validateCertificate)
   at Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.AddressRegistrationTests.RegisterAddresses_Success(String addressInput, String[] testUrls, Int32 testPort) in /_/test/Kestrel.FunctionalTests/AddressRegistrationTests.cs:line 179
   at Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.AddressRegistrationTests.RegisterAddresses_IPv6_Success(String addressInput, String[] testUrls) in /_/test/Kestrel.FunctionalTests/AddressRegistrationTests.cs:line 121

benaadams · 2018-02-03T23:40:35Z

Reworded commit to retrigger CI

davidfowl · 2018-02-04T16:21:00Z

src/Kestrel.Core/Internal/Infrastructure/HttpUtilities.cs

+                throw new ArgumentException(nameof(value));
+            }
+
+            var firstChar = value[0];


Why this implementation if its not performance sensitive? Just use the http abstractions methods

Is used by http2 so may be perf sensitive :-/

Also are aspnet dlls ngen'd in the newest versions? Does mean none of the methods can be inlined if so.

Did you mean

if (HttpMethods.IsGet(value)) { method = HttpMethod.Get; return true; }

How is this used by HTTP/2? If this is only hit by applications setting the request method themselves, I too would prefer a more readable slightly slower version of this method.

HTTP/2 goes via string header

((IHttpRequestFeature)this).Method = RequestHeaders[":method"];

Though it probably shouldn't use an interface cast for it https://github.com/dotnet/coreclr/issues/16198

i.e. first line of Http2Stream.TryParseRequest(ReadResult result, out bool endConnection)

Improved the TryParseRequest not to cast via interface and use method directly

Also changed to use the HttpMethods strings

halter73 · 2018-02-05T21:15:39Z

src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs

+                    return _customMethod;
+                }
+
+                _customMethod = HttpUtilities.MethodToString(Method);


The old code did HttpUtilities.MethodToString(method) ?? string.Empty. I'd either keep that or have MethodToString throw instead of return null neither of which should ever actually happen in practice.

halter73 · 2018-02-05T21:18:33Z

src/Kestrel.Core/Internal/Infrastructure/HttpUtilities.cs

+                throw new ArgumentException(nameof(value));
+            }
+
+            var firstChar = value[0];


How is this used by HTTP/2? If this is only hit by applications setting the request method themselves, I too would prefer a more readable slightly slower version of this method.

Tratcher · 2018-02-05T17:53:16Z

src/Kestrel.Core/Internal/Http/HttpProtocol.cs

@@ -324,7 +325,8 @@ public void Reset()
            MaxRequestBodySize = ServerOptions.Limits.MaxRequestBodySize;
            AllowSynchronousIO = ServerOptions.AllowSynchronousIO;
            TraceIdentifier = null;
-            Method = null;
+            Method = HttpMethod.Get;


HttpMethod.None

Had to change None = byte.MaxValue as so indexing and hashing uses the values

Tratcher · 2018-02-05T21:51:08Z

src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs

+                    return _customMethod;
+                }
+
+                _customMethod = HttpUtilities.MethodToString(Method);


Why store this in _costomMethod? It's a static lookup. You store null for Set.

GetKnownMethod returns an HttpMethod enum for known methods, but not the string so null.

The field is there anyway for a non-known method, so once you've looked it up once, might as well store it if its checked again rather than doing another lookup,

So why didn't you store the value in the Set code path?

If you're going to use it for both then call it _methodText

Ahh.. that's a very good point 😄

JamesNK · 2018-02-06T06:45:18Z

src/Kestrel.Core/Internal/Infrastructure/HttpUtilities.cs

+        /// The Known Methods (CONNECT, DELETE, GET, HEAD, PATCH, POST, PUT, OPTIONS, TRACE)
+        /// </remarks>
+        /// <returns><c>true</c> if the input matches a known string, <c>false</c> otherwise.</returns>
+        public static bool GetKnownMethod(string value, out HttpMethod method)


Should this be called TryGetKnownMethod? It is following the try pattern of returning a bool with an out param.

Reverted, always returns value in out

What's the bool for then?

Ah good point can clean this up; return the string

benaadams · 2018-02-06T16:00:09Z

K, done - I think

davidfowl · 2018-02-06T16:02:32Z

src/Kestrel.Core/Internal/Infrastructure/HttpUtilities.cs

                return method != HttpMethod.Custom;
            }
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal static unsafe HttpMethod GetKnownMethod(byte* data, int length, out int methodLength)
+        internal static unsafe HttpMethod TryGetKnownMethod(byte* data, int length, out int methodLength)


This shouldn't be Try

Because it always returns a value in out?

Reverted

benaadams · 2018-02-06T16:08:17Z

K, now done... hopefully :)

JamesNK · 2018-02-06T20:55:29Z

src/Kestrel.Core/Internal/Infrastructure/HttpUtilities.cs

+        /// The Known Methods (CONNECT, DELETE, GET, HEAD, PATCH, POST, PUT, OPTIONS, TRACE)
+        /// </remarks>
+        /// <returns><c>true</c> if the input matches a known string, <c>false</c> otherwise.</returns>
+        public static string GetKnownMethod(string value, out HttpMethod method)


This method is taking string value and then returning it back again. Why not just return HttpMethod instead of an out param?

Also XML docs need to be updated.

halter73 · 2018-02-16T23:55:48Z

src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs

+            }
+            set
+            {
+                Method = HttpUtilities.GetKnownMethod(value);


Out of curiosity, what would happen if we treated every request method set by the app via IHttpRequestFeature.Method as a custom method whether or not it known in order to elide this call to GetKnownMethod?

I don't see how this could be a problem, because none of Kestrel's logic should be predicated on the request method set by the application.

So the app couldn't change request behavior based on method? If you changed method from GET -> HEAD the would mean if the app produced no body, but a content length it would throw with an 500 error.

However that may be better than responding with a HEAD response to a GET request and then hanging the browser while waiting for data that would never come?

While the app should be able to change all of the properties of HttpContext to lie to itself, the server must maintain an internal source of truth so it can properly interact with the client.

This change is really messing up the tests :(

benaadams · 2018-02-17T06:21:59Z

Changed and rebased

benaadams · 2018-02-23T02:08:36Z

Rebased

halter73

Thanks! LGTM.

halter73 · 2018-02-23T02:47:57Z

@pakrym you want to take a look?

davidfowl · 2018-02-23T17:24:56Z

@benaadams can you rebase?

davidfowl · 2018-02-23T17:26:32Z

src/Kestrel.Core/Internal/Infrastructure/HttpUtilities.cs

+        /// <returns><see cref="HttpMethod"/></returns>
+        public static HttpMethod GetKnownMethod(string value)
+        {
+            // Called by http/2 and if user code overrides and sets the Method on IHttpRequestFeature


This is only used for http2 now, not setting the method directly right?

Yes, will change comment

benaadams · 2018-02-23T17:38:58Z

Rebased, changed comment

davidfowl

LGTM

halter73 · 2018-02-23T22:29:49Z

Thanks!

sebastienros · 2018-02-25T18:22:56Z

7% improvement on plaintext running on our physical.

benaadams · 2018-02-25T18:25:28Z

😄 What does the CF mean on those graphs?

sebastienros · 2018-02-25T18:31:25Z

Old benchmark to analyze the cost of connection adapters.

Used here: https://github.com/aspnet/benchmarks/blob/dev/src/Benchmarks/Program.cs#L234
With this filter: https://github.com/aspnet/benchmarks/blob/dev/src/Benchmarks/PassthroughConnectionFilter.cs#L12

halter73 · 2018-02-26T01:54:39Z

This really shows what a difference one extra string comparison per request makes. I think (object.ReferenceEquals(context.ServerSetMethod, HttpMethods.Head)) would have also worked, but moving to enum comparisons was much cleaner.

Thanks @benaadams for finding and fixing this.

benaadams · 2018-02-26T02:10:17Z

Don't think its just down to this one as there were 5 PRs merged at the same time; but glad they had an effect 😄

Use enum for method rather than string compares (#2294)
Sanitize and centralize exception throws (#2293)
Flatten exception handling (#2313)
Speed up TryGetAsciiString (#1973)
Faster IFeatureCollection.Get (#2290)

benaadams · 2018-02-26T02:12:15Z

Also none of them in the area of recent regression, so everything still to play for 😉

davidfowl requested review from halter73, davidfowl and Tratcher February 3, 2018 23:24

davidfowl reviewed Feb 3, 2018

View reviewed changes

benaadams force-pushed the enum-method branch from 3176df2 to 22fb267 Compare February 3, 2018 23:40

davidfowl reviewed Feb 4, 2018

View reviewed changes

halter73 suggested changes Feb 5, 2018

View reviewed changes

Tratcher reviewed Feb 5, 2018

View reviewed changes

JamesNK reviewed Feb 6, 2018

View reviewed changes

davidfowl reviewed Feb 6, 2018

View reviewed changes

JamesNK reviewed Feb 6, 2018

View reviewed changes

benaadams force-pushed the enum-method branch from 12bed8c to a009bc1 Compare February 6, 2018 21:32

muratg added this to the 2.1.0-preview2 milestone Feb 13, 2018

muratg assigned halter73 Feb 13, 2018

halter73 suggested changes Feb 16, 2018

View reviewed changes

benaadams force-pushed the enum-method branch from a009bc1 to 1c6d82d Compare February 17, 2018 06:21

benaadams force-pushed the enum-method branch 3 times, most recently from 35d5e16 to 7c528d4 Compare February 23, 2018 01:38

halter73 approved these changes Feb 23, 2018

View reviewed changes

davidfowl reviewed Feb 23, 2018

View reviewed changes

Use enum for method rather than string compare

1ced1a3

benaadams added 3 commits February 23, 2018 17:36

Feedback

143abc3

Fix tests

2f584ae

Fix comment

b0e3650

benaadams force-pushed the enum-method branch from 7c528d4 to b0e3650 Compare February 23, 2018 17:38

davidfowl approved these changes Feb 23, 2018

View reviewed changes

pakrym approved these changes Feb 23, 2018

View reviewed changes

halter73 merged commit de7e2a2 into aspnet:dev Feb 23, 2018

benaadams deleted the enum-method branch February 26, 2018 02:06

benaadams mentioned this pull request Jan 31, 2020

Do interface casts to access methods elide the virtual dispatch? dotnet/runtime#9658

Closed

Use enum for method rather than string compares #2294

Use enum for method rather than string compares #2294

Conversation

benaadams commented Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Feb 3, 2018

benaadams commented Feb 3, 2018

davidfowl Feb 4, 2018 • 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

benaadams Feb 5, 2018 • 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

Choose a reason for hiding this comment

Tratcher Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Feb 6, 2018 • 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

benaadams commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Tratcher Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Feb 17, 2018

benaadams commented Feb 23, 2018

halter73 left a comment

Choose a reason for hiding this comment

halter73 commented Feb 23, 2018

davidfowl commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Feb 23, 2018

davidfowl left a comment

Choose a reason for hiding this comment

halter73 commented Feb 23, 2018

sebastienros commented Feb 25, 2018

benaadams commented Feb 25, 2018

sebastienros commented Feb 25, 2018

halter73 commented Feb 26, 2018

benaadams commented Feb 26, 2018

benaadams commented Feb 26, 2018

benaadams commented Feb 3, 2018 •

edited

Loading

davidfowl Feb 4, 2018 •

edited

Loading

benaadams Feb 5, 2018 •

edited

Loading

Tratcher Feb 5, 2018 •

edited

Loading

JamesNK Feb 6, 2018 •

edited

Loading

benaadams Feb 17, 2018 •

edited

Loading

Tratcher Feb 17, 2018 •

edited

Loading