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.

Sign up for GitHub

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

Jump to bottom

Reduce some allocations in SslStream handshake. #103814

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Reduce some allocations in SslStream handshake. #103814

merged 2 commits into from
Jun 24, 2024

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Jun 21, 2024 •
edited
Loading

This removes some unnecessary allocations on TLS handshakes, mostly on Linux.


BenchmarkDotNet v0.13.13-nightly.20240311.145, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores
.NET SDK 9.0.100-preview.5.24307.3
  [Host]     : .NET 9.0.0 (9.0.24.30607), X64 RyuJIT AVX2
  Job-QJUXKM : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  Job-OFJRMC : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

PowerPlanMode=00000000-0000-0000-0000-000000000000  IterationTime=250ms  MaxIterationCount=20  
MinIterationCount=15  WarmupCount=1
Method Job Toolchain Mean Error StdDev Median Min Max Ratio RatioSD Allocated Alloc Ratio
DefaultHandshakeContextIPv4Async Job-QJUXKM /9.0.0/corerun 1.089 ms 0.0267 ms 0.0297 ms 1.083 ms 1.048 ms 1.146 ms 1.03 0.03 5.81 KB 0.98
DefaultHandshakeContextIPv4Async Job-OFJRMC /main/corerun 1.058 ms 0.0147 ms 0.0137 ms 1.059 ms 1.034 ms 1.079 ms 1.00 0.02 5.96 KB 1.00
DefaultMutualHandshakeContextIPv4Async Job-QJUXKM /9.0.0/corerun 1.432 ms 0.0217 ms 0.0181 ms 1.432 ms 1.395 ms 1.459 ms 1.01 0.02 8.06 KB 0.75
DefaultMutualHandshakeContextIPv4Async Job-OFJRMC /main/corerun 1.424 ms 0.0262 ms 0.0269 ms 1.430 ms 1.362 ms 1.468 ms 1.00 0.03 10.73 KB 1.00
DefaultHandshakeIPv4Async Job-QJUXKM /9.0.0/corerun 6.375 ms 0.3801 ms 0.3904 ms 6.319 ms 5.923 ms 7.178 ms 1.07 0.07 9.67 KB 1.00
DefaultHandshakeIPv4Async Job-OFJRMC /main/corerun 5.961 ms 0.1159 ms 0.1084 ms 5.973 ms 5.755 ms 6.219 ms 1.00 0.02 9.69 KB 1.00
DefaultMutualHandshakeIPv4Async Job-QJUXKM /9.0.0/corerun 11.515 ms 0.4361 ms 0.5022 ms 11.386 ms 10.935 ms 12.586 ms 1.02 0.05 14.56 KB 0.84
DefaultMutualHandshakeIPv4Async Job-OFJRMC /main/corerun 11.323 ms 0.2131 ms 0.1994 ms 11.356 ms 11.020 ms 11.657 ms 1.00 0.02 17.37 KB 1.00

rzikm
rzikm commented Jun 21, 2024
View reviewed changes
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs Outdated
Comment on lines 162 to 171
byte[]? hash = null;

if (sslAuthenticationOptions.CertificateContext?.TargetCertificate is X509Certificate2 cert)
{
// This is equivalent to cert.GetCertHash(HashAlgorithmName.Sha256), but this
// way the code does not allocate a new byte[] with raw cert contents.every time
hash = SHA256.HashData(cert.RawDataMemory.Span);
}

var key = new SslContextCacheKey(protocols, hash);
Copy link
Member Author

@rzikm rzikm Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@bartonjs is the X509Certificate.GetCertHash(...) behavior by design? looks like something that could potentially help in multiple places if we can change the implementation to use the internaly cached RawData.

Copy link
Member

Choose a reason for hiding this comment

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

(Not Jeremy, but...) I don't think so. We can probably make some improvements here.

Copy link
Member

Choose a reason for hiding this comment

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

Since it's using certPal.RawData instead of this.RawData, I think it's trying to avoid the allocation; but clearly some PALs are caching that and others are not, because we started off with a weak PAL contract and haven't ever cleaned it up.

In this case, it looks like just switching from certPal.RawData to this.RawDataMemory.Span will mean we know exactly what caching is involved.

Copy link
Member

Choose a reason for hiding this comment

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

With #103828 merged I am not sure this is needed anymore.

@rzikm rzikm requested a review from wfurt June 21, 2024 12:37
stephentoub
stephentoub reviewed Jun 21, 2024
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs
@@ -149,7 +149,7 @@ internal void UpdateOptions(SslServerAuthenticationOptions sslServerAuthenticati

private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols protocols)
{
if (protocols.HasFlag(SslProtocols.Tls12) || protocols.HasFlag(SslProtocols.Tls13))
if ((protocols & (SslProtocols.Tls12 | SslProtocols.Tls13)) != SslProtocols.None)
Copy link
Member

Choose a reason for hiding this comment

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

The change is fine, but did you see this allocating?

Copy link
Member Author

@rzikm rzikm Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I collected a trace via dotnet trace and saw an entry for this. The signature is apparently

public bool HasFlag (Enum flag);

so I assume it is boxing the argument. The trace was in Debug configuration, I am not sure if it happens in Release as well.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it happens in Release as well.

The JIT special-cases HasFlag. It shouldn't be allocating in optimized code.

Copy link
Member

Choose a reason for hiding this comment

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

The trace was in Debug configuration

You generally don't want to do any perf analysis based on debug. Optimizations are disabled, both at the JIT level and also the C# level (e.g. async methods get emitted as classes rather than structs), so it provides a skewed view of the world.

Copy link
Member Author

@rzikm rzikm Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I got unsure and rechecked the trace, it indeed comes from a different place. But I don't see it in the most recent traces for some reason.

@vcsjones vcsjones mentioned this pull request Jun 21, 2024
Merged
This was referenced Jun 21, 2024
Open
Closed
@build-analysis build-analysis bot mentioned this pull request Jun 24, 2024
Open
@rzikm rzikm merged commit 03f58cb into dotnet:main Jun 24, 2024
80 of 83 checks passed
@karelz karelz added this to the 9.0.0 milestone Jun 24, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bartonjs bartonjs bartonjs left review comments

@stephentoub stephentoub stephentoub approved these changes

@vcsjones vcsjones vcsjones left review comments

Assignees

@rzikm rzikm

Labels
area-System.Net.Security
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@rzikm @vcsjones @stephentoub @bartonjs @karelz