HTTP/3: ValueTask pooling #42760
HTTP/3: ValueTask pooling #42760
Conversation
JamesNK
requested review from
Tratcher,
halter73 and
BrennanConroy
as code owners
|
Do we have any data indicating that pooling here is beneficial? |
|
I'll benchmark before/after once other PRs are merged and profile performance again. I doubt this change alone will have any material difference. |
|
Before: After: |
JamesNK
force-pushed
the
jamesnk/valuetaskpooling
branch
from
91df2ac to
d462577
Compare
| @@ -371,7 +379,7 @@ private async Task DoSend() | |||
| // A client can abort a stream after it has finished sending data. We need a way to get that notification | |||
| // which is why we listen for a notification that the write-side of the stream is done. | |||
| // An exception can be thrown from the stream on client abort which will be captured and then wake up the output read. | |||
| _ = WaitForWritesCompleted(); | |||
| var waitForWritesClosedTask = WaitForWritesClosedAsync(); | |||
There was a problem hiding this comment.
Bad merge? This undoes #42683
There was a problem hiding this comment.
It's intentional. I noticed some failures when benchmarking after that change.
Also, pooling the task requires awaiting it.
There was a problem hiding this comment.
_ = for Tasks and ValueTasks is almost always bad. I've seen very few counterexamples. Although it's often done for expedience when properly handling the task would be extremely difficult, it would still be better to await everything if it were easily doable.
Even if you do things like log all uncaught exceptions to make discarding tasks better, how do you know the logger is still alive sending your logs to wherever they need to go if you aren't awaiting all your tasks and properly delaying shutdown?
ghost
added this to the 
amcasey
added
area-networking
Pool some async methods on QuicStreamContext.