Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls #11942

benaadams · 2019-07-07T15:23:56Z

if _pipeReader.TryRead fails to get the data synchronously defer to an IAsyncEnumerable<int> that is allocated once on the first failure. (Use case WebSockets/SignalR over TLS)

Before

After

I used an IAsyncEnumerable as then the code very closely mimics the original read loop; other than adding yield before the returns and a slightly modified behaviour for the finally clause.

Resolves: #11940

/cc @stephentoub @davidfowl @halter73

benaadams · 2019-07-07T15:33:32Z

Hide whitespace shows the change in the main methods better https://github.com/aspnet/AspNetCore/pull/11942/files?w=1

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs

stephentoub · 2019-07-08T00:31:11Z

What's the single-threaded CPU overhead of this approach?

src/Servers/Kestrel/Core/src/Middleware/Internal/DuplexPipeStream.cs

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs

src/Servers/Kestrel/Core/src/Internal/Infrastructure/AsyncEnumerableReader.cs

benaadams · 2019-07-11T15:45:06Z

What's the single-threaded CPU overhead of this approach?

Looks good; it only engages the AsynEnumerable when there is no data and goes back to sync when there is data:

InMemoryTransportBenchmark

              Method |     Mean |     Error |     StdDev |   Median |     Op/s | Allocated |
 ------------------- |---------:|----------:|-----------:|---------:|---------:|----------:|
-          Plaintext | 84.22 us | 3.2848 us |  9.1020 us | 86.95 us | 11,874.2 |     352 B |
+          Plaintext | 79.11 us | 3.8055 us | 10.8573 us | 83.59 us | 12,641.1 |     348 B |
- PlaintextPipelined | 11.52 us | 0.2011 us |  0.1783 us | 11.46 us | 86,778.7 |     368 B |
+ PlaintextPipelined | 10.50 us | 0.2045 us |  0.1913 us | 10.46 us | 95,220.7 |     355 B |

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs

jkotalik · 2019-07-11T16:37:14Z

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs


-                        return actual;
+                        // Finally blocks in enumerators aren't excuted prior to the yield return,
+                        // so we advance here


Hm... that seems like it will be a bug farm in the future.

I don't think this is that bad. The finally still runs given an uncaught exception or the completion of the method. I think it would be more weird if the finally did run for every yield return.

src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs

benaadams · 2019-07-13T21:42:01Z

Changed to use ManualResetValueTaskSourceCore as it makes it a bit simplier

benaadams · 2019-07-18T13:14:34Z

Rebased

src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs

halter73 · 2019-07-18T22:27:16Z

@davidfowl @jkotalik Can you also take a look at this PR? It LGTM.

benaadams · 2019-08-20T03:02:51Z

Rebased

benaadams · 2019-08-20T03:55:45Z

@halter73 #12639 fixed the counting, but still needs the timing change

Error message
Moq.MockException : \nExpected invocation on the mock should never have been performed, but was 32 times: h => h.OnTimeout(It.IsAny<TimeoutReason>())\r\n\r\nConfigured setups: \r\nITimeoutHandler h => h.OnTimeout(It.IsAny<TimeoutReason>())\r\n\r\nPerformed invocations: \r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)\r\nITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate)

Stack trace
   at Moq.Mock.VerifyCalls(Mock targetMock, InvocationShape expectation, LambdaExpression expression, Times times, String failMessage)
   at Moq.Mock.Verify[T](Mock`1 mock, Expression`1 expression, Times times, String failMessage)
   at Moq.Mock`1.Verify(Expression`1 expression, Times times)
   at Moq.Mock`1.Verify(Expression`1 expression, Func`1 times)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Tests.Http2TimeoutTests.DATA_Received_TooSlowlyOnSecondStream_AbortsConnectionAfterNonAdditiveRateTimeout() in /_/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs:line 775
--- End of stack trace from previous location where exception was thrown ---

benaadams · 2019-08-20T18:23:09Z

src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs

-            var result = _context.RequestBodyPipe.Reader.TryRead(out readResult);
-            _readResult = readResult;
-            CountBytesRead(readResult.Buffer.Length);
+            TryStart();


@halter73 note: had to include this Http2MessageBody change for TryStart/TryStop to prevent test failures.

It definitely feels better to call TryStart here. It does important stuff that isn't even timing related like checking that the request body size isn't larger than what's allowed.

What test failures were you seeing specifically.

One example was the following though there were a couple different types

Error message Moq.MockException : Expected invocation on the mock should never have been performed, but was 32 times: h => h.OnTimeout(It.IsAny<TimeoutReason>()) Configured setups: ITimeoutHandler h => h.OnTimeout(It.IsAny<TimeoutReason>()) Performed invocations: ITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate) ITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate) ITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate) ITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate) ITimeoutHandler.OnTimeout(TimeoutReason.ReadDataRate) ... Stack trace at Moq.Mock.VerifyCalls(Mock targetMock, InvocationShape expectation, LambdaExpression expression, Times times, String failMessage) at Moq.Mock.Verify[T](Mock`1 mock, Expression`1 expression, Times times, String failMessage) at Moq.Mock`1.Verify(Expression`1 expression, Times times) at Moq.Mock`1.Verify(Expression`1 expression, Func`1 times) at Microsoft.AspNetCore.Server.Kestrel.Core.Tests.Http2TimeoutTests.DATA_Received_TooSlowlyOnSecondStream_AbortsConnectionAfterNonAdditiveRateTimeout() in /_/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs:line 775 --- End of stack trace from previous location where exception was thrown ---

Ah. That makes sense. If you start reading a request body with TryRead, and then call ReadAsync, Kestrel will reset _readTimingBytesRead to 0 on the first ReadAsync call ignoring the bytes counted by TryRead.

I wonder how common using both TryRead and ReadAsync on the same RequestBody will be before we do it implicitly ReadAsyncInternal as part of this PR. We might want to backport this part of the change.

Does Http1ContentLengthMessageBody need this?

Http1ChunkedEncodingMessageBody has it; but ContentLength doesn't have the same pattern (TryRead earlier and TryStop)

Ah; is correct TryStop is in CreateReadResultFromConnectionReadResult

Added isolated change for Http2 #13305

benaadams · 2019-08-22T14:10:46Z

Probably want to hold on this for now until dotnet/coreclr#26310 is evaluated

stephentoub · 2019-08-22T14:12:47Z

Probably want to hold on this for now until dotnet/coreclr#26310 is evaluated

I'll welcome your help evaluating it :)

analogrelay · 2019-11-05T21:48:36Z

I asked @halter73 what the deal is here and he said "oooh, this is a spicy one, but now that we're in 5.0, we can look at it". 😉

@davidfowl @halter73 Let's re-open this now that 5.0 is open
@benaadams can you rebase?

halter73 · 2019-11-06T01:44:21Z

dotnet/coreclr#26310 did get merged, so adding IAsyncEnumerable<int> ReadAsyncAwaited probably doesn't make sense any more.

benaadams · 2019-11-06T01:53:31Z

Agreed; but it still needs to be evaluated as my be backed out https://github.com/dotnet/coreclr/issues/27403

/cc @sebastienros

stephentoub · 2019-11-06T15:00:12Z

My hope is that the functionality from #27403 remains and ends up being enabled by default in .NET 5. But I need help from folks in vetting that and coming back with perf numbers from workloads to justify it.

Even if that ships on-by-default, though, there's potentially value in optimizing critical cases with hand-rolled implementations, if doing so can, for example, do a more efficient job of caching than the built-in caching. And as any such hand-rolled implementation involves a ton more code, it would need to be so much better that it was worth the maintenance / bug risk that is likely in such complicated code. I don't know if this particular case / implementation meets that bar.

stephentoub · 2019-11-06T15:01:14Z

(btw, I've implemented an analyzer to help flag cases of ValueTask misuse that can cause problems as more and more such caching is applied: dotnet/roslyn-analyzers#3001)

analogrelay · 2019-11-20T21:57:55Z

@benaadams can you rebase to resolve conflicts?

@halter73 @jkotalik are we good to merge this once conflicts resolved (and failing builds either kicked into submission or fixed ;))

davidfowl · 2019-11-21T03:26:14Z

We should make the pipelines change @benaadams

halter73 · 2019-11-21T21:46:39Z

We should make the pipelines change @benaadams

What changes exactly are you referring to?

analogrelay · 2020-01-06T22:24:56Z

@benaadams @davidfowl @halter73 dotnet/coreclr#26310 has been merged now. Do we still want this change? I'd like to clean it up if not.

benaadams · 2020-01-06T22:43:00Z

No it isn't needed currently.

analogrelay · 2020-01-07T00:22:32Z

Sweet! Thanks!

benaadams requested review from analogrelay, halter73, jkotalik and Tratcher as code owners July 7, 2019 15:23

davidfowl reviewed Jul 7, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs Outdated Show resolved Hide resolved

benaadams force-pushed the HttpRequestStream.ReadAsync- branch from 6eb9771 to 3b40f05 Compare July 8, 2019 00:24

benaadams force-pushed the HttpRequestStream.ReadAsync- branch from 28e2e14 to 86032fb Compare July 8, 2019 13:15

mkArtakMSFT added the area-servers label Jul 8, 2019

analogrelay added this to the 3.0.0-preview8 milestone Jul 8, 2019

halter73 reviewed Jul 8, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Middleware/Internal/DuplexPipeStream.cs Show resolved Hide resolved

halter73 reviewed Jul 8, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Middleware/Internal/DuplexPipeStream.cs Show resolved Hide resolved

halter73 reviewed Jul 8, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs Outdated Show resolved Hide resolved

halter73 reviewed Jul 8, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs Outdated Show resolved Hide resolved

benaadams changed the title ~~Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls~~ [WIP] Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls Jul 10, 2019

benaadams force-pushed the HttpRequestStream.ReadAsync- branch from 86032fb to 3b5f879 Compare July 11, 2019 14:25

benaadams changed the title ~~[WIP] Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls~~ Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls Jul 11, 2019

davidfowl reviewed Jul 11, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/AsyncEnumerableReader.cs Outdated Show resolved Hide resolved

jkotalik reviewed Jul 11, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestStream.cs Show resolved Hide resolved

jkotalik reviewed Jul 11, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs Show resolved Hide resolved

benaadams force-pushed the HttpRequestStream.ReadAsync- branch from 4dd5bde to 38690bc Compare July 13, 2019 21:41

benaadams force-pushed the HttpRequestStream.ReadAsync- branch from 4af0241 to 2835575 Compare July 18, 2019 13:14

halter73 reviewed Jul 18, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs Outdated Show resolved Hide resolved

halter73 approved these changes Jul 18, 2019

View reviewed changes

pranavkm added this to the 5.0.0-alpha1 milestone Aug 13, 2019

benaadams force-pushed the HttpRequestStream.ReadAsync- branch from 33b7347 to ca00257 Compare August 20, 2019 02:57

Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls

9137a7f

benaadams force-pushed the HttpRequestStream.ReadAsync- branch from ca00257 to 9137a7f Compare August 20, 2019 14:23

benaadams commented Aug 20, 2019

View reviewed changes

benaadams mentioned this pull request Aug 21, 2019

Http2MessageBody.TryRead TryStart/TryStop #13305

Closed

benaadams mentioned this pull request Aug 23, 2019

Make "async ValueTask/ValueTask<T>" methods ammortized allocation-free dotnet/coreclr#26310

Merged

analogrelay modified the milestones: blazor-wasm, 5.0.0-preview1 Nov 5, 2019

analogrelay assigned halter73 Nov 5, 2019

halter73 self-requested a review November 6, 2019 18:19

benaadams closed this Jan 6, 2020

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

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls #11942

Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls #11942

benaadams commented Jul 7, 2019 •

edited

Loading

benaadams commented Jul 7, 2019

stephentoub commented Jul 8, 2019

benaadams commented Jul 11, 2019

jkotalik Jul 11, 2019

halter73 Jul 18, 2019

benaadams commented Jul 13, 2019

benaadams commented Jul 18, 2019

halter73 commented Jul 18, 2019

benaadams commented Aug 20, 2019

benaadams commented Aug 20, 2019

benaadams Aug 20, 2019

halter73 Aug 20, 2019

benaadams Aug 20, 2019 •

edited

Loading

halter73 Aug 20, 2019

benaadams Aug 20, 2019

benaadams Aug 21, 2019

benaadams commented Aug 22, 2019

stephentoub commented Aug 22, 2019

analogrelay commented Nov 5, 2019

halter73 commented Nov 6, 2019

benaadams commented Nov 6, 2019

stephentoub commented Nov 6, 2019

stephentoub commented Nov 6, 2019

analogrelay commented Nov 20, 2019

davidfowl commented Nov 21, 2019

halter73 commented Nov 21, 2019

analogrelay commented Jan 6, 2020

benaadams commented Jan 6, 2020

analogrelay commented Jan 7, 2020

Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls #11942

Amortize HttpRequestStream & DuplexPipeStream ReadAsync calls #11942

Conversation

benaadams commented Jul 7, 2019 • edited Loading

benaadams commented Jul 7, 2019

stephentoub commented Jul 8, 2019

benaadams commented Jul 11, 2019

jkotalik Jul 11, 2019

Choose a reason for hiding this comment

halter73 Jul 18, 2019

Choose a reason for hiding this comment

benaadams commented Jul 13, 2019

benaadams commented Jul 18, 2019

halter73 commented Jul 18, 2019

benaadams commented Aug 20, 2019

benaadams commented Aug 20, 2019

benaadams Aug 20, 2019

Choose a reason for hiding this comment

halter73 Aug 20, 2019

Choose a reason for hiding this comment

benaadams Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

halter73 Aug 20, 2019

Choose a reason for hiding this comment

benaadams Aug 20, 2019

Choose a reason for hiding this comment

benaadams Aug 21, 2019

Choose a reason for hiding this comment

benaadams commented Aug 22, 2019

stephentoub commented Aug 22, 2019

analogrelay commented Nov 5, 2019

halter73 commented Nov 6, 2019

benaadams commented Nov 6, 2019

stephentoub commented Nov 6, 2019

stephentoub commented Nov 6, 2019

analogrelay commented Nov 20, 2019

davidfowl commented Nov 21, 2019

halter73 commented Nov 21, 2019

analogrelay commented Jan 6, 2020

benaadams commented Jan 6, 2020

analogrelay commented Jan 7, 2020

benaadams commented Jul 7, 2019 •

edited

Loading

benaadams Aug 20, 2019 •

edited

Loading