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
avoid ProtocolToken allocations in TLS handshake #86163
Conversation
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones Issue DetailsProtocolToken are somewhat high for 1000 TLS handshakes: Since that is basically just container for status and buffet it seems unnecessary. There may be further optimizations but for now I turn it into AFAIK this is last low hanging fruit -> closes #68951
|
@@ -31,13 +31,13 @@ private JavaProxy.RemoteCertificateValidationResult VerifyRemoteCertificate() | |||
}; | |||
} | |||
|
|||
private bool TryGetRemoteCertificateValidationResult(out SslPolicyErrors sslPolicyErrors, out X509ChainStatusFlags chainStatus, out ProtocolToken? alertToken, out bool isValid) | |||
private bool TryGetRemoteCertificateValidationResult(out SslPolicyErrors sslPolicyErrors, out X509ChainStatusFlags chainStatus, ref ProtocolToken alertToken, out bool isValid) |
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.
Nit: A bunch of these look like they could remain as out
(e.g., NextMessage
, ProcessTlsFrame
)
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
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.
Other than the pending comments, LGTM.
ProtocolToken are somewhat high for 1000 TLS handshakes:
Since that is basically just container for status and buffet it seems unnecessary. There may be further optimizations but for now I turn it into
struct
and pass ref to it. To pass that across async boundaryReceiveBlobAsync
no longer calls crypto e.g. it does only IO andProcessTlsFrame
(former ProcessBlob) is called explicit to process it.AFAIK this is last low hanging fruit -> closes #68951