Skip to content

Improve HTTP/1 response header parsing - #74393

Merged
MihaZupan merged 8 commits into
dotnet:mainfrom
MihaZupan:http-response-headers-perf
Oct 13, 2022
Merged

Improve HTTP/1 response header parsing#74393
MihaZupan merged 8 commits into
dotnet:mainfrom
MihaZupan:http-response-headers-perf

Conversation

@MihaZupan

@MihaZupanMihaZupan commented Aug 23, 2022

Copy link
Copy Markdown
Member

Based on the "portable" implementation from @scalablecory's PR #63295.
This PR does not include the Avx2-vectorized parsing code.

Sending in-memory (no I/O) requests and parsing the response:

MethodToolchainResponseHeadersMeanErrorStdDevMedianRatioRatioSD
SendAsyncmain11.701 μs0.0075 μs0.0373 μs1.689 μs1.000.00
SendAsyncpr11.481 μs0.0014 μs0.0071 μs1.480 μs0.870.02
SendAsyncmain21.880 μs0.0085 μs0.0428 μs1.863 μs1.000.00
SendAsyncpr21.674 μs0.0044 μs0.0225 μs1.673 μs0.890.02
SendAsyncmain32.129 μs0.0037 μs0.0184 μs2.126 μs1.000.00
SendAsyncpr31.751 μs0.0018 μs0.0092 μs1.749 μs0.820.01
SendAsyncmain42.302 μs0.0050 μs0.0248 μs2.296 μs1.000.00
SendAsyncpr41.883 μs0.0044 μs0.0218 μs1.886 μs0.820.02
SendAsyncmain83.257 μs0.0049 μs0.0246 μs3.258 μs1.000.00
SendAsyncpr82.647 μs0.0114 μs0.0577 μs2.628 μs0.810.02
SendAsyncmain165.871 μs0.0129 μs0.0666 μs5.882 μs1.000.00
SendAsyncpr164.472 μs0.0226 μs0.1145 μs4.409 μs0.760.02
SendAsyncmain3211.995 μs0.0314 μs0.1611 μs11.958 μs1.000.00
SendAsyncpr329.302 μs0.0153 μs0.0783 μs9.306 μs0.780.01
SendAsyncmain6427.980 μs0.0543 μs0.2808 μs27.870 μs1.000.00
SendAsyncpr6422.456 μs0.0496 μs0.2555 μs22.358 μs0.800.01

@ghostghost assigned MihaZupanAug 23, 2022
@ghost

ghost commented Aug 23, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Based on the "portable" implementation from @scalablecory's PR #63295.
This PR does not include the Avx2-vectorized parsing code.

Sending in-memory (no I/O) requests and parsing the response:

MethodToolchainResponseHeadersMeanErrorStdDevMedianRatioRatioSD
SendAsyncmain11.701 μs0.0075 μs0.0373 μs1.689 μs1.000.00
SendAsyncpr11.481 μs0.0014 μs0.0071 μs1.480 μs0.870.02
SendAsyncmain21.880 μs0.0085 μs0.0428 μs1.863 μs1.000.00
SendAsyncpr21.674 μs0.0044 μs0.0225 μs1.673 μs0.890.02
SendAsyncmain32.129 μs0.0037 μs0.0184 μs2.126 μs1.000.00
SendAsyncpr31.751 μs0.0018 μs0.0092 μs1.749 μs0.820.01
SendAsyncmain42.302 μs0.0050 μs0.0248 μs2.296 μs1.000.00
SendAsyncpr41.883 μs0.0044 μs0.0218 μs1.886 μs0.820.02
SendAsyncmain83.257 μs0.0049 μs0.0246 μs3.258 μs1.000.00
SendAsyncpr82.647 μs0.0114 μs0.0577 μs2.628 μs0.810.02
SendAsyncmain165.871 μs0.0129 μs0.0666 μs5.882 μs1.000.00
SendAsyncpr164.472 μs0.0226 μs0.1145 μs4.409 μs0.760.02
SendAsyncmain3211.995 μs0.0314 μs0.1611 μs11.958 μs1.000.00
SendAsyncpr329.302 μs0.0153 μs0.0783 μs9.306 μs0.780.01
SendAsyncmain6427.980 μs0.0543 μs0.2808 μs27.870 μs1.000.00
SendAsyncpr6422.456 μs0.0496 μs0.2555 μs22.358 μs0.800.01
Author:MihaZupan
Assignees:-
Labels:

area-System.Net.Http

Milestone:-

@MihaZupan
MihaZupan requested a review from a teamAugust 23, 2022 01:10
@MihaZupan

MihaZupan commented Aug 23, 2022

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Aug 23, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
MihaZupanforce-pushed the http-response-headers-perf branch from 6e51403 to 7804b82CompareSeptember 6, 2022 01:53
rzikm
rzikm approved these changes Sep 6, 2022
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@MihaZupan

MihaZupan commented Sep 7, 2022

Copy link
Copy Markdown
MemberAuthor

Test failure is #74795

@MihaZupan

MihaZupan commented Sep 7, 2022

Copy link
Copy Markdown
MemberAuthor

@stephentoub could you please take a look at this one if you get a chance, given there was quite a bit of discussion on the original PR introducing these sorts of changes (#63295).

stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
stephentoub
stephentoub reviewed Sep 7, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated
@MihaZupan

MihaZupan commented Sep 26, 2022

Copy link
Copy Markdown
MemberAuthor

@dotnet/ncl can someone please take a look at this one?

CarnaViire
CarnaViire approved these changes Sep 27, 2022
View reviewed changes

@CarnaViireCarnaViire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but it will be better if @stephentoub could take another look...

@MihaZupanMihaZupan added this to the 8.0.0 milestone Oct 10, 2022
stephentoub
stephentoub reviewed Oct 11, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated

@stephentoubstephentoubOct 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this check < but the one at https://github.com/dotnet/runtime/pull/74393/files#diff-595f700a4c83b735d856fe14b5ebe8c7f7c18ae6e586adc8b398a997a0678274R997 which may also be comparing against buffer.Length uses <=?

@MihaZupanMihaZupanOct 11, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

When we check <= above, it's after we've determined that we must read more data in order to make progress. If we're already equal to the limit, there's no point in trying to read again as we'll definitely go over the limit.

Here we check < because we may already be done with reading. If the response headers happened to consume exactly _allowedReadLineBytes, we don't throw as we technically didn't go over the limit.
More accurately, this check could be

if(finished?(_allowedReadLineBytes<bytesConsumed):(_allowedReadLineBytes<=buffer.Length))

though I don't think it matters either way. Both of the checks could be < or <= and we'd just be playing with +/- 1 on the limit (which is controlled in KB anyway).

stephentoub
stephentoub reviewed Oct 11, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs Outdated

@stephentoubstephentoubOct 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like a really slow way to trim ending whitespace. Do we not expect any?

@stephentoubstephentoubOct 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Also it's only looking for ' '... other whitespace doesn't matter?)

@MihaZupanMihaZupanOct 11, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The expected hot path is that there are no trailing spaces.

(Also it's only looking for ' '... other whitespace doesn't matter?)

This matches the existing behavior.

According to the spec, the name shouldn't have trailing whitespace at all
field-line = field-name ":" OWS field-value OWS

We also trim the whitespace around the value. We trim leading SP and HTAB, but only trailing SP.

I think it's fine that we're more accepting on the name, but it's odd that we differ for leading/trailing OWS on the value.

Opened #77001

@MihaZupan
MihaZupanforce-pushed the http-response-headers-perf branch from b62e86b to 7be0a23CompareOctober 11, 2022 16:00
This was referenced Oct 11, 2022
Closed
Closed
@build-analysisbuild-analysisBot mentioned this pull request Oct 12, 2022
Closed
2 tasks
@MihaZupan
MihaZupan merged commit d2d5ad3 into dotnet:mainOct 13, 2022
@MihaZupanMihaZupan mentioned this pull request Oct 13, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Nov 12, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes
@CarnaViireCarnaViireCarnaViire approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Http

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@MihaZupan@stephentoub@CarnaViire@rzikm