Skip to content
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

Merged
merged 3 commits into from Jun 4, 2020

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Jun 3, 2020

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. (@geoffkizer, this is the change you and I discussed offline; should make it easier for you to try out your ideas.)

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

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.
@stephentoub stephentoub changed the title Change Http2Connection.StartWriteAsync to use a callback model … Change Http2Connection.StartWriteAsync to use a callback model Jun 3, 2020
@ghost
Copy link

ghost commented Jun 3, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@stephentoub stephentoub added the tenet-performance Performance related issue label Jun 3, 2020
@stephentoub stephentoub added this to the 5.0 milestone Jun 3, 2020
Copy link
Contributor

@scalablecory scalablecory left a 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;
Copy link
Contributor

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?

@davidfowl
Copy link
Member

davidfowl commented Jun 3, 2020

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));
    }
}

@stephentoub
Copy link
Member Author

Random style comment, to avoid closures (assuming that's a goal), I've adopted a pattern that introduces static local functions to avoid captures:

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).

@davidfowl
Copy link
Member

@jaredpar are we close? 😄

@stephentoub
Copy link
Member Author

dotnet/csharplang#275

@jaredpar
Copy link
Member

jaredpar commented Jun 3, 2020

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.

@stephentoub
Copy link
Member Author

Excellent, @jaredpar and @jcouv. Excited for it.

@stephentoub stephentoub merged commit b2eaf5c into dotnet:master Jun 4, 2020
@stephentoub stephentoub deleted the http2invert branch June 4, 2020 00:49
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants