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

pool async state in SslStream #69418

Merged
merged 4 commits into from
May 26, 2022
Merged

pool async state in SslStream #69418

merged 4 commits into from
May 26, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented May 17, 2022

fixes #68467

I had simple test code like

   using (var sslClient = new SslStream(_client, leaveInnerStreamOpen: true, delegate { return true; }))
   using (var sslServer = new SslStream(_server, leaveInnerStreamOpen: true, delegate { return true; }))
   {

       await Task.WhenAll(
           sslClient.AuthenticateAsClientAsync(clientOptions),
           sslServer.AuthenticateAsServerAsync(serverOptions));

       for (int j = 0; j < 100; j++)
       {
           var t = sslClient.ReadAsync(_clientBuffer, CancellationToken.None);
           await sslServer.WriteAsync(_serverBuffer, CancellationToken.None);
           await t;
       }
  }

before the change it would allocate

image

after the change

image

EnsureFullTlsFrameAsync is also used in handshake so it should help little with Connection: close as well.

@wfurt wfurt requested review from stephentoub and a team May 17, 2022 04:38
@wfurt wfurt self-assigned this May 17, 2022
@ghost
Copy link

ghost commented May 17, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #68467

I had simple test code like

   using (var sslClient = new SslStream(_client, leaveInnerStreamOpen: true, delegate { return true; }))
   using (var sslServer = new SslStream(_server, leaveInnerStreamOpen: true, delegate { return true; }))
   {

       await Task.WhenAll(
           sslClient.AuthenticateAsClientAsync(clientOptions),
           sslServer.AuthenticateAsServerAsync(serverOptions));

       for (int j = 0; j < 100; j++)
       {
           var t = sslClient.ReadAsync(_clientBuffer, CancellationToken.None);
           await sslServer.WriteAsync(_serverBuffer, CancellationToken.None);
           await t;
       }
  }

before the change it would allocate

image

after the change

image

EnsureFullTlsFrameAsync is also used in handshake so it should help little with Connection: close as well.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security

Milestone: -

@stephentoub
Copy link
Member

EnsureFullTlsFrameAsync is also used in handshake so it should help little with Connection: close as well.

If you want to use it here, go ahead. But FWIW I think this case is a little more suspect. Pooling the read method objects seems fairly reasonable, as in general we expect the SslStreams themselves to be relatively long-lived, pooled themselves as part of a connection pool. One of the difficulties with object pooling is that it can create artificial references from gen2 to gen0, and so if the state you're using is itself likely to be gen2, this negative aspect of pooling mostly goes away. With Connection: close, though, that suggests this is a scenario where these streams aren't being pooled and are much less likely to be gen2, and as such it's more likely that pooling the state machines will end up creating gen2 to gen0 references.

@wfurt
Copy link
Member Author

wfurt commented May 17, 2022

EnsureFullTlsFrameAsync is also used in handshake so it should help little with Connection: close as well.

If you want to use it here, go ahead. But FWIW I think this case is a little more suspect.

The EnsureFullTlsFrameAsync as also called from Read so it is on hot path. I did not touch methods that are only in handshake e.g. called once per session. There are still easier wins for handshake.

@rzikm
Copy link
Member

rzikm commented May 24, 2022

The changeset now seems to include #69527, was this intentional?

@wfurt
Copy link
Member Author

wfurt commented May 25, 2022

no, it was not intentional @rzikm.

@davidfowl
Copy link
Member

Nice! Are the other allocations the handshake?

@wfurt
Copy link
Member Author

wfurt commented May 26, 2022

There are some @davidfowl. Some are easy to avoid some of them are more tricky.

It would be great if you can share any real workload you care about. With that we can investigate allocations as well as other runtime parts.

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfurt
Copy link
Member Author

wfurt commented May 26, 2022

Failing Quic tests are unrelated and will be fixed by #69709 (msquic upgrade)

@wfurt wfurt merged commit 008f128 into dotnet:main May 26, 2022
@wfurt wfurt deleted the asyncPool branch May 26, 2022 13:28
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PoolingAsyncValueTaskMethodBuilder to System.Net.Security.SslStream.ReadAsyncInternal()
5 participants