Make HTTP/1.1 startline parsing "safe"#20885
Merged
halter73 merged 3 commits intodotnet:masterfrom Apr 24, 2020
Merged
Conversation
Member
Author
|
@aspnet-hello benchmark |
|
Starting 'Default' pipelined plaintext benchmark with session ID '56b8c19e4bba4094b5e2fa8c684ae99d'. This could take up to 30 minutes... |
BaselinePR |
davidfowl
reviewed
Apr 16, 2020
davidfowl
reviewed
Apr 16, 2020
Member
Author
There was a problem hiding this comment.
HttpVersionAndMethod contains 3 parts
int MethodEnd { get; set; }
HttpVersion Version { get; }
HttpMethod Method { get; }To fit in a long HttpMethod can't be an int
Member
Author
There was a problem hiding this comment.
Any api changes are all to .Internal ones
86ee27b to
c9d75f5
Compare
Member
Author
|
@aspnet-hello benchmark |
Member
Random IIS test failure (bad cert) on the previous run. |
|
Starting 'Default' pipelined plaintext benchmark with session ID 'a9961ac6b9dd4df8a020c1ddaecc14e0'. This could take up to 30 minutes... |
BaselinePR |
Member
Author
|
Still good 🙂 if larger 😢 |
c50117a to
a89d1f5
Compare
a89d1f5 to
e088a5f
Compare
Member
Author
|
RequestParsingBenchmark Http1ConnectionBenchmark |
Member
Author
|
@aspnet-hello benchmark |
adamsitnik
reviewed
Apr 22, 2020
Member
|
@benaadams could you please run the JSON microbenchmark as well? |
Member
Author
Made the Results: |
Member
Author
|
Error seems to be build out of memory issue? |
Member
Member
Member
@benaadams Awesome, thank you! It means that if we were spending 5% of total time in parsing, with the 20% improvement we are going to get an extra 1% gain in RPS, which should translate to 10k as of today. |
halter73
reviewed
Apr 22, 2020
| [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]public TargetOffsetPathLength(int offset, int length, bool isEncoded) { throw null; } | ||
| public bool IsEncoded { [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]get { throw null; } } | ||
| public int Length { [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]get { throw null; } } | ||
| public int Offset { [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]get { throw null; } } |
Member
There was a problem hiding this comment.
@davidfowl @rynowak The HttpParser and associated types are the last remaining "pubternal" APIs in Kestrel. They are used in our TE "platform" benchmarks. Is it okay to bypass API review for this?
Member
There was a problem hiding this comment.
We do need to design an API though....
Member
There was a problem hiding this comment.
What? You don't love TargetOffsetPathLength!?
Member
Author
There was a problem hiding this comment.
I assume he means to expose it as external api?
|
Starting 'Default' pipelined plaintext benchmark with session ID '4606a59255514add9011ac69204820a6'. This could take up to 30 minutes... |
BaselinePR |
halter73
approved these changes
Apr 22, 2020
Member
halter73
left a comment
There was a problem hiding this comment.
This is awesome! Both "safe" PRs have been really good. It's nice to prove we don't need pointers to be fast anymore.
Member
Author
|
Platform change for this PR in aspnet/benchmarks aspnet/Benchmarks#1497 |
Member
|
Is there some way we can give dotnet/runtime the ability to run asp.net benchmarks like you're doing here? |
Member
Author
I assume you mean like the request to @aspnet-hello benchmark ? |
Member
|
@AndyAyersMS there are many ways to interpret this, please chat with me offline |
Member
|
@AndyAyersMS everyone should have it, it's amazing 😄 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To @blowdart with ❤
Contributes to #4720
HttpParserBenchmark
RequestParsingBenchmark
Http1ConnectionBenchmark
Note: this will require a corresponding change to the Platform versions due to change in internal api
Resolves: #20518
/cc @davidfowl @adamsitnik