-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Change Http2Connection.StartWriteAsync to use a callback model #37353
Conversation
In all of the places we use StartWriteAsync, it's followed by some amount of synchronous work and then releasing the lock. We can instead pass that synchronous work into StartWriteAsync as a callback. This has a few benefits: 1. If/when StartWriteAsync completes asynchronously, in most of the call sites we don't incur another async method then completing asynchronously and allocating its state machine, or needing to call through another layer of state machines. 2. We can have spans as locals inside of the callbacks as they're not async methods, which lets us reduce the number of times we access Memory.Span. 3. We can more easily experiment with different execution models around invoking the work waiting for the lock.
Tagging subscribers to this area: @dotnet/ncl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just some curiosities.
|
||
EndWrite(forceFlush: false); | ||
_outgoingBuffer.Commit(bytesWritten); | ||
_lastPendingWriterShouldFlush |= flush == FlushTiming.AfterPendingWrites; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost certainly inconsequential, but do CPUs optimize out this load/store if the value being ORed is 0?
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
Random style comment, to avoid closures (assuming that's a goal), I've adopted a pattern that introduces static local functions to avoid captures: class Something
{
void Foo()
{
static void DoFoo(Something thisRef, int value)
{
// No captures in here, static local function
}
// Value tuple as state and lambda calling DoFoo to get static caching
FooWithCallback((thisRef: this, something: value), state => DoFoo(state.thisRef, state.value));
}
} |
I'm hopeful we'll have static lambdas soon to address this :) Your solution is a good workaround. Primary downside to it is, at least today, the delegate can't be inlined, and there's a really good chance the static local function won't be inlineable either (in most of the cases in this PR), so it's introducing another non-inlineable function call. That may or may not be measurable (probably wouldn't be here in most cases here). |
@jaredpar are we close? 😄 |
There is an implementation available here https://github.com/dotnet/roslyn/tree/features/static-lambdas. It hasn't merged back yet just cause we've been pushing on other features. This should still be doable for C# 9. |
In all of the places we use StartWriteAsync, it's followed by some amount of synchronous work and then releasing the lock. We can instead pass that synchronous work into StartWriteAsync as a callback. This has a few benefits:
On my machine, this looks to improve https://github.com/jamesnk/http2perf with "r 100 false" client arguments by ~10%.
Contributes to #35184
cc: @geoffkizer, @scalablecory