-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
improve TLS perf on macOS #32338
improve TLS perf on macOS #32338
Conversation
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.
Looks good. Small notes.
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
It looks like there is more optimization opportunity here if you bypass the |
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
Show resolved
Hide resolved
With #32925 in place, handshake, EncryptMessage and DecyptMessage are all mutually exclusive. The way how it flows is that managed code read TLS frame and than we put it to _inputBuffer using context.Write(). Then we call OS crypto function and that consumes the data as needed. When data needs to be sent out, WriteToConnection() puts it to _outputBuffer and when the call returns we collect that using ReadPendingWrites(). In this whole process there is no concurrency so the locks are not needed any more. This should be ready for final reviews. |
@@ -14,11 +14,12 @@ namespace System.Net | |||
{ | |||
internal sealed class SafeDeleteSslContext : SafeDeleteContext | |||
{ | |||
private const int InitialBufferSize = 2048; |
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.
How often are these SafeDeleteSslContexts created? 4K seems fairly large (with two of these 2K sizes allocated below).
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.
they are created once for each SslStream. TLS frame can be up to 16k but I wanted to fit at least typical handshake. Some other parts for SslStream would release the buffer every time when remaining data drop to zero. I can possibly do that as part of this change or as follow-up. With OpenSSL we simply write data to BIO and magic happens. But I don';t think it shrinks the buffers - I can take closer look as well.
Thos removes old data processing and uses ArrayBuffers instead. I think the lock is not needed, but keeping it in place has a negligible impact so it seems safer. I may do more testing with upcoming locking changes to support TLS13.
The biggest winner (no surprise) is LargeBuffer test with 189us -> 35us.
Old Code:
This change: