Skip to content

Don't double-flush HTTP/1 Content-Length responses #11825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mkArtakMSFT merged 2 commits into from
Jul 3, 2019
Merged

Don't double-flush HTTP/1 Content-Length responses #11825

mkArtakMSFT merged 2 commits into from
Jul 3, 2019

Conversation

@halter73
Copy link
Member

@halter73 halter73 commented Jul 2, 2019

This should fix the biggest single cause of the perf regression discussed in #11778. The regression was introduced by #11675 which caused PipeWriter.FlushAsync() to be called twice per response in the TechEmpower plaintext benchmark instead of once like before.

I fixed this by checking whether _unflushedBytes > 0 in Http1OutputProducer.WriteStreamSuffixAsync() for non-chunked responses. If there are no remaining unflushed bytes, we now skip the redundant flush.

@halter73
Copy link
Member Author

halter73 commented Jul 2, 2019

Here are some before and after numbers from the Pipelined Plaintext TechEmpower benchmark:

Before PR

RequestsPerSecond:           4,701,874
Max CPU (%):                 94
WorkingSet (MB):             54
Avg. Latency (ms):           1.39
Startup (ms):                349
First Request (ms):          97.18
Latency (ms):                0.11
Total Requests:              70,998,775
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
SDK:                         3.0.100-preview7-012769
Runtime:                     3.0.0-preview7-27825-08
ASP.NET Core:                3.0.0-preview7.19328.3

After PR

RequestsPerSecond:           5,018,954
Max CPU (%):                 90
WorkingSet (MB):             53
Avg. Latency (ms):           1.46
Startup (ms):                366
First Request (ms):          96.96
Latency (ms):                0.12
Total Requests:              75,784,120
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
SDK:                         3.0.100-preview7-012769
Runtime:                     3.0.0-preview7-27825-08
ASP.NET Core:                3.0.0-preview7.19328.3

So it looks like we got about ~6.5% of our RPS back in this benchmark.

jkotalik
jkotalik reviewed Jul 2, 2019
View reviewed changes
jkotalik
jkotalik reviewed Jul 2, 2019
View reviewed changes
jkotalik
jkotalik reviewed Jul 2, 2019
View reviewed changes
@halter73 halter73 changed the base branch from master to release/3.0-preview7 July 3, 2019 01:44
@mkArtakMSFT
Copy link
Contributor

mkArtakMSFT commented Jul 3, 2019

@jkotalik can you please sign off on this, if you feel comfortable with the fix? Thanks!

@jkotalik
Copy link
Contributor

jkotalik commented Jul 3, 2019

Is WebApiTemplateAsync a known flaky test? I've seen it fail once here https://github.com/aspnet/AspNetCore-Internal/issues/2220 but Template failures scare me as they showed problems in the past.

@halter73
Copy link
Member Author

halter73 commented Jul 3, 2019 •
edited
Loading

The template tests have caught some HTTP/2-related issues. Fortunately, this change is HTTP/1-only where we have much better test coverage.

@mkArtakMSFT
Copy link
Contributor

mkArtakMSFT commented Jul 3, 2019

Merging as this is approved for Preview 7!

@mkArtakMSFT mkArtakMSFT merged commit 503ef7d into release/3.0-preview7 Jul 3, 2019
@ghost ghost deleted the halter73/11778 branch July 3, 2019 17:58
@mkArtakMSFT
Copy link
Contributor

mkArtakMSFT commented Jul 3, 2019

@dougbu this was approved for Preview 7 release. Please make sure this is included in the build we produce.

@dougbu
Copy link
Contributor

dougbu commented Jul 3, 2019

We don't have a coherent build yet but @wtgodbe is aware of this and started a couple of internal builds containing the fix to shorten the time-to-BAR

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidfowl davidfowl davidfowl approved these changes

@Tratcher Tratcher Tratcher approved these changes

@mkArtakMSFT mkArtakMSFT Awaiting requested review from mkArtakMSFT

@analogrelay analogrelay Awaiting requested review from analogrelay

+1 more reviewer

@jkotalik jkotalik jkotalik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@halter73 @mkArtakMSFT @jkotalik @dougbu @davidfowl @Tratcher @amcasey