Streamline SocketHttpHandler's ParseStatusLine validation #27163
Conversation
For a typical status line like "HTTP/1.1 200 OK", cuts the time of ParseStatusLine almost in half.
| @@ -36,6 +36,8 @@ public Version Version | |||
| } | |||
| } | |||
|
|
|||
| internal void SetVersionUnchecked(Version value) => _version = value; | |||
There was a problem hiding this comment.
The equivalent API for headers is called "TryAddWithoutValidation". Not sure this is better than just "Unchecked", but maybe we should be consistent.
| @@ -635,73 +637,78 @@ private CancellationTokenRegistration RegisterCancellation(CancellationToken can | |||
| // TODO: Remove this overload once https://github.com/dotnet/roslyn/issues/17287 is addressed | |||
| // and the compiler doesn't lift the span temporary from the call site into the async state | |||
| // machine in debug builds. | |||
| private void ParseStatusLine(ArraySegment<byte> line, HttpResponseMessage response) => | |||
| private static void ParseStatusLine(ArraySegment<byte> line, HttpResponseMessage response) => | |||
There was a problem hiding this comment.
Since you're in this code anyway :), can we remove this overload per comment above?
There was a problem hiding this comment.
Unfortunately, no, the C# compiler still prevents span locals in an async method. @VSadov, is there an open issue tracking that I can link to? The overall issue I linked to above is closed.
| else | ||
| { | ||
| byte minorVersion = line[7]; | ||
| line[7] = (byte)'1'; // Enable checking the first 7 bytes easily by comparing against HTTP/1.1 |
There was a problem hiding this comment.
It's a little weird to modify the buffer. This code isn't perf critical, would it be better to just do something dumb here?
There was a problem hiding this comment.
Yeah, went back and forth on that. It shouldn't matter that we're modifying it, but I could do the "simpler" thing and just add the per-char comparisons here... unfortunately that simpler thing is actually longer, but whatever 😄
There was a problem hiding this comment.
Yeah... to make it smaller, we could possibly just have a static byte[] for "HTTP/1." and do some sort of sequence based comparison (is there a Span.SequenceEquals?)
There was a problem hiding this comment.
There is. I'll do that.
| ThrowInvalidHttpResponse(); | ||
| Span<byte> reasonBytes = line.Slice(MinStatusLineLength + 1); | ||
| string knownReasonPhrase = HttpStatusDescription.Get(response.StatusCode); | ||
| response.SetReasonPhraseUnchecked(knownReasonPhrase != null && EqualsOrdinal(knownReasonPhrase, reasonBytes) ? |
There was a problem hiding this comment.
Seems like we should still validate unknown reason phrases -- either add the logic here specifically and throw on failure, or just call the checked API.
See #26946 also, which is probably a good argument for adding the logic here.
There was a problem hiding this comment.
Seems like we should still validate unknown reason phrases
We already did. The only thing it validates is that it doesn't contain \r or \n and that it's not null, and none of those could be the case here.
There was a problem hiding this comment.
Can we reenable the test for #26946 and close the issue, then?
There was a problem hiding this comment.
Actually, I'm wrong. I thought our line parsing was looking for \r, but it's now looking for \n, which means a \r could still be here. I'll make the unknown phrase still use the checking logic.
There was a problem hiding this comment.
(but I'll fix the issue with that test and re-enable it anyway)
|
LGTM |
|
how did you measure the improvement @stephentoub ??? |
I temporarily modified the binary to expose HttpConnection.ParseStatusLine as a public method. Then I wrote a little microbenchmark against that. |
|
thanks for the explanation. |
|
😄 |
|
None of the failures are real; they're all do the test infrastructure problems we've been facing the last few days. |
* Streamline SocketHttpHandler's ParseStatusLine validation For a typical status line like "HTTP/1.1 200 OK", cuts the time of ParseStatusLine almost in half. * Address PR feedback
…efx#27163) * Streamline SocketHttpHandler's ParseStatusLine validation For a typical status line like "HTTP/1.1 200 OK", cuts the time of ParseStatusLine almost in half. * Address PR feedback Commit migrated from dotnet/corefx@c8d23b4
For a typical status line like "HTTP/1.1 200 OK", cuts the time of ParseStatusLine almost in half.
Fixes https://github.com/dotnet/corefx/issues/26946
cc: @geoffkizer, @davidsh