HTTP/3: ValueTask pooling #42760
HTTP/3: ValueTask pooling #42760
Conversation
|
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: |
91df2ac to
d462577
Compare
| // 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.
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?
Pool some async methods on QuicStreamContext.