Remove a few allocations from SslStream #22304
Conversation
Each call is allocating a delegate for the Begin and End method. We can avoid that by passing `this` in as state.
| else | ||
| { | ||
| if (ClientCertificates.Count == 0) | ||
| if (_clientCertificates == null || _clientCertificates.Count == 0) |
There was a problem hiding this comment.
Would it be a good idea to flip the if and change to if(_clientCertificates?.Count > 0) ?
There was a problem hiding this comment.
Why would that be better?
There was a problem hiding this comment.
Since the if and else have code in common, the common could be moved out of the branch to
if (NetEventSource.IsEnabled) NetEventSource.Log.NoDelegateNoClientCert(this);
if (ClientCertificates.Count == 0)
{
sessionRestartAttempt = true;
}(Assuming NoDelegateNoClientCert doesn't side-effect on ClientCertificates which I haven't checked. Not a big impact, but if one was going to refactor the if, I'd say do that rather than flipping it.
There was a problem hiding this comment.
You are right (never attempt a review on a mobile phone). So I am looking at the code on my computer now, this isn't the method I am really worried about. It's more the next one AcquireClientCredentials that is many pages long, and nested a number of levels deep. It looks a bit "openssl". So I have no problem with the changes in general, just wondering if while this is being looked at if this could be refactored in some way. Anyway the changes you have made look good and are an improvement.
There was a problem hiding this comment.
Ok, thanks. I wasn't planning to do any refactoring as part of this change. There's plenty of it that can and should be done later :)
|
LGTM |
* Remove unnecessary X509CertificateCollection allocations * Avoid delegate allocations per AuthenticateAs call in SslStream Each call is allocating a delegate for the Begin and End method. We can avoid that by passing `this` in as state.
* Remove unnecessary X509CertificateCollection allocations * Avoid delegate allocations per AuthenticateAs call in SslStream Each call is allocating a delegate for the Begin and End method. We can avoid that by passing `this` in as state. Commit migrated from dotnet/corefx@96dbafb
6 participants
Minor, as they only affect the AuthenticateAsClient/Server paths, which generally happen only once per stream, but every little bit helps:
thisin as instance state.cc: @geoffkizer, @Drawaes, @CIPop