decrease allocations during ssl handshake

[wfurt]

The test I'm using is essentially handshake test from Performance repo with one round of setup and 1000 test iterations:

for (int i = 0; i < max; i++)
{
    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("localhost", null, SslProtocols.None, checkCertificateRevocation: false),
        sslServer.AuthenticateAsServerAsync(_cert, clientCertificateRequired: false, SslProtocols.None, checkCertificateRevocation: false));

        // Workaround for corefx#37765
        await sslServer.WriteAsync(_serverBuffer, CancellationToken.None);
        await sslClient.ReadAsync(_clientBuffer, CancellationToken.None);     
    }
}

When running with OpenSSL we get following allocations:

There are two parts in this change:

ProtocolToken is container to hold status and buffer. It can be allocated once and reused. That brings 5 allocation to 1. We may possibly convert it to struct in future.

We already use shared buffers for encryption.decryption. Wit this change we will also use it for outgoing handshake messages when openssl is used.

[stephentoub]

I'd feel ever so slightly better if this were instead:

byte[] tmp = sendBuf;
sendBuf = null;
ArrayPool<byte>.Shared.Return(tmp);

If, for example, Crypto.ErrClearError were to throw an exception, with the code as written we could have returned the buffer to the pool but the caller could still have a reference via the out argument that was already assigned.

[stephentoub]

Can we just make ProtocolToken a readonly struct?

[stephentoub]

Ooh, yeah, this definitely makes me nervous. Erroneous setting of Payload could now result in the same buffer being returned multiple times to the pool, e.g. if we accidentally access it concurrently. And it'll be worse if we do make ProtocolToken a struct (maybe that's why you chose not to).

Does _payload need to actually be a separate buffer? We already have _internalBuffer on the SslStream... can we just use that?

[wfurt]

I was wondering about it but the ProtocolToken use is currently pretty simple and controlled by us. I originally did not have it IDisposable and I was worrying about loosing rented buffer in error cases and causing memory leaks. I call Reset() when we are done with token to release buffer as soon as possible. We can possibly assert or throw if buffer is not null when assigning new value. That would make sure we don't mingle usages.

ProtocolToken is IMHO convenience wrapper and we could possibly abandon it and track status and buffers separately. That would make it easier to track life cycle.

I was also thinking about using _internalBuffer. Handshake happens before we would normally use it and we rent buffer big enough for whole TLS record. The trouble would be handling renegotiation. It still may be possible but the _internalBuffer could have chunk of data while we would start new handshake. We could use same strategy on separate variable and keep the rented buffer in class variable instead of holding to it via ProtocolToken.

Let me know what seems most appropriate.

[wfurt]

I'll find another solution and I'll open new PR when ready.