Skip to content

Reuse HTTP2 stream pipes to reduce allocations #19356

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
merged 2 commits into from
Mar 5, 2020
Merged

Reuse HTTP2 stream pipes to reduce allocations #19356

merged 2 commits into from
Mar 5, 2020

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Feb 26, 2020

Fixes #19276

@ghost ghost added the area-servers label Feb 26, 2020
@JamesNK
Copy link
Member Author

JamesNK commented Feb 26, 2020 •
edited
Loading

GONE:

image

@JamesNK JamesNK mentioned this pull request Feb 26, 2020
Merged
@analogrelay analogrelay added this to the 5.0.0-preview2 milestone Feb 26, 2020
@analogrelay
Copy link
Contributor

PR Benchmark? @halter73 ? I think we have an http2 one now.

Tratcher
Tratcher reviewed Feb 26, 2020
View reviewed changes
@JamesNK
Copy link
Member Author

JamesNK commented Feb 26, 2020

@aspnet-hello benchmark http2

@pr-benchmarks
Copy link

pr-benchmarks bot commented Feb 26, 2020

Starting 'http2' pipelined plaintext benchmark with session ID 'f587ddda7fd541c7ab34bd6f96cfa9ed'. This could take up to 30 minutes...

@pr-benchmarks
Copy link

pr-benchmarks bot commented Feb 26, 2020

Baseline

Starting baseline run on '4e18bca0a69e360fe7653e4e49bd689997d1e10a'...
RequestsPerSecond:           106,339
Max CPU (%):                 93
WorkingSet (MB):             211
Avg. Latency (ms):           7.38
Startup (ms):                487
First Request (ms):          219.11
Latency (ms):                0.79
Total Requests:              1,595,091
Duration: (ms)               15,010
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             21,013
Published Size (KB):         119,146
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.2.20120.8
ASP.NET Core:                5.0.0-preview.2.20125.12

PR

Starting PR run on '9bcdbf012b0c9ea438668fb94cd1add492443c31'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 106,339 |      93 |         211 |              7.38 |          487 |           21013 |              119146 |             219.11 |         0.79 |      0 |  1.00 |
|       After | 108,194 |      94 |         223 |              6.19 |          478 |            7510 |              119146 |             186.54 |         0.92 |      0 |  1.02 |

@jkotalik jkotalik mentioned this pull request Feb 26, 2020
Closed
@jkotalik
Copy link
Contributor

I had some feedback with regards to whether we are allowed to reuse a stream here: #19360. Let's see if my hypothesis is correct, and if so, we should be able to use BeginRequestProcessing as another place to reset the Http2OutputProcuder/Pipe.

halter73
halter73 previously requested changes Feb 26, 2020
View reviewed changes
@JamesNK JamesNK mentioned this pull request Feb 26, 2020
Merged
@JamesNK JamesNK force-pushed the jamesnk/http2-pipe-reuse branch from 9bcdbf0 to f127560 Compare February 27, 2020 08:28
@JamesNK
Copy link
Member Author

JamesNK commented Feb 27, 2020

@aspnet-hello benchmark http2

@pr-benchmarks
Copy link

pr-benchmarks bot commented Feb 27, 2020

Starting 'http2' pipelined plaintext benchmark with session ID 'b8d774b98cad4c4fba20d75206a84278'. This could take up to 30 minutes...

@pr-benchmarks
Copy link

pr-benchmarks bot commented Feb 27, 2020

Baseline

Starting baseline run on '5b7672c2adf9733bb84222dd403725d35b17d835'...
RequestsPerSecond:           108,070
Max CPU (%):                 93
WorkingSet (MB):             213
Avg. Latency (ms):           10.64
Startup (ms):                495
First Request (ms):          210.8
Latency (ms):                0.57
Total Requests:              1,621,053
Duration: (ms)               15,000
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             18,014
Published Size (KB):         119,145
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.2.20125.16
ASP.NET Core:                5.0.0-preview.2.20126.7

PR

Starting PR run on 'ffaae1394ba57a6a2319402ebfe5a23415e9590b'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 108,070 |      93 |         213 |             10.64 |          495 |           18014 |              119145 |              210.8 |         0.57 |      0 |  1.00 |
|       After | 108,195 |      93 |         197 |              5.71 |          506 |            7515 |              119145 |             200.59 |          0.6 |      0 |  1.00 |

jkotalik
jkotalik reviewed Feb 28, 2020
View reviewed changes
src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs

_pipeWriter = new ConcurrentPipeWriter(pipe.Writer, pool, _dataWriterLock);
_pipeReader = pipe.Reader;
_pipeWriter = new ConcurrentPipeWriter(_pipe.Writer, _memoryPool, _dataWriterLock);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually surprised that ConcurrentPipeWriter doesn't show up in your profile. I guess there aren't many scenarios where it would allocate.

Copy link
Contributor

Choose a reason for hiding this comment

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

For comparison, how much did it allocate before?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see it here:

image

@JamesNK JamesNK force-pushed the jamesnk/http2-pipe-reuse branch from 1d5b809 to bcbaef4 Compare March 2, 2020 18:59
jkotalik
jkotalik reviewed Mar 3, 2020
View reviewed changes
@JamesNK
Copy link
Member Author

JamesNK commented Mar 3, 2020

Before:

|       Method |     Mean |     Error |    StdDev |      Op/s |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------- |---------:|----------:|----------:|----------:|-------:|------:|------:|----------:|
| EmptyRequest | 4.219 us | 0.0876 us | 0.1789 us | 236,995.5 | 0.0458 |     - |     - |   3.31 KB |

After:

|       Method |     Mean |     Error |    StdDev |      Op/s |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------- |---------:|----------:|----------:|----------:|-------:|------:|------:|----------:|
| EmptyRequest | 3.944 us | 0.0841 us | 0.2440 us | 253,528.5 | 0.0305 |     - |     - |   2.02 KB |

@JamesNK JamesNK force-pushed the jamesnk/http2-pipe-reuse branch from bcbaef4 to a19e489 Compare March 3, 2020 20:26
@JamesNK JamesNK force-pushed the jamesnk/http2-pipe-reuse branch from 8e2a154 to 32a91be Compare March 4, 2020 00:10
@JamesNK JamesNK force-pushed the jamesnk/http2-pipe-reuse branch from b4504d9 to 43391fb Compare March 5, 2020 01:39
@JamesNK JamesNK requested a review from halter73 March 5, 2020 19:14
@JamesNK JamesNK dismissed halter73’s stale review March 5, 2020 19:15

Changes applied

@JamesNK JamesNK merged commit 0db0640 into master Mar 5, 2020
@JamesNK JamesNK deleted the jamesnk/http2-pipe-reuse branch March 5, 2020 19:15
@halter73 halter73 mentioned this pull request May 26, 2020
Merged
@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 left review comments

@Tratcher Tratcher Tratcher left review comments

@analogrelay analogrelay Awaiting requested review from analogrelay

@halter73 halter73 Awaiting requested review from halter73

+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
5.0.0-preview2
Development

Successfully merging this pull request may close these issues.

HTTP2: Reuse request and response Pipe
7 participants
@JamesNK @analogrelay @jkotalik @halter73 @davidfowl @Tratcher @amcasey