Enable ArrayPool buffer pooling in Apple/Android SslStream PALs#130601
Enable ArrayPool buffer pooling in Apple/Android SslStream PALs#130601artl93 wants to merge 3 commits into
Conversation
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Pull request overview
Enables ProtocolToken payload buffer pooling (ArrayPool<byte>) for the Apple (SecureTransport) and Android SslStream PAL implementations, aligning them with the existing Windows and Unix/OpenSSL PAL behavior and reducing steady-state TLS allocation churn.
Changes:
- Set
token.RentBuffer = truein Apple and Android PALs forEncryptMessageandHandshakeInternal. - Add a functional regression test that performs repeated variable-size writes and validates round-trip integrity.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs | Enables pooled ProtocolToken payload buffers for encrypted-write and handshake output tokens on Apple PAL. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs | Enables pooled ProtocolToken payload buffers for encrypted-write and handshake output tokens on Android PAL. |
| src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamStreamToStreamTest.cs | Adds regression coverage for repeated variable-size application-data writes with end-to-end integrity validation. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
|
I think the macOS and Android was never in center of the attention and I feel our test coverage is not that great. Renting comes with higher risk of leaks where the buffers need to be managed explicitly and correctly instead of relying on GC. But in some ways that ship has already sailed. The best thing would be eliminate the intermediate buffers on platforms where we can. Something that come up recently within our team and something I had PoC for long time ago ... but never finished. |
…ndroid PALs SslStreamPal.OSX.cs and SslStreamPal.Android.cs never set ProtocolToken.RentBuffer = true in EncryptMessage/HandshakeInternal, unlike the Windows and OpenSSL (Unix) PALs which both do. As a result, every encrypted write and every handshake-produced output token on Apple (macOS/iOS/tvOS/Mac Catalyst) and Android allocated a fresh `new byte[Size]` via ProtocolToken.SetPayload/EnsureAvailableSpace instead of renting from ArrayPool<byte>.Shared, sized to the ciphertext (record) length. Fix: set token.RentBuffer = true immediately after `ProtocolToken token = default;` in both EncryptMessage and HandshakeInternal in each PAL, mirroring the existing Windows/Unix pattern exactly. ReleasePayload() already checks the per-token RentBuffer flag before returning to the pool, so this is a drop-in, no-risk activation of already-existing, already-tested pooling logic - no new code paths. Measured on macOS (Apple Secure Transport PAL), exact-parent A/B (same commit 7aa830a, GC.GetTotalAllocatedBytes(true), warmed up, 20000 iters record/handshake, 2000 iters/worker pooled): TLS1.2 handshake: 48053.8 -> 46379.8 B/op (-3.5%) mTLS handshake: 74049.0 -> 71274.7 B/op (-3.7%) record round-trip 1B: 1028.2 -> 959.3 B/op (-6.7%) record round-trip 1KiB: 3126.6 -> 970.7 B/op (-69.0%) record round-trip 16KiB:33857.6 -> 977.8 B/op (-97.1%) record round-trip 64KiB:134116.5 -> 2709.3 B/op (-98.0%) pooled c=1, 4KiB: 9435.4 -> 1013.9 B/op (-89.3%, prior-session baseline) pooled c=16, 4KiB: 9323.6 -> 987.6 B/op (-89.4%, prior-session baseline) pooled c=64, 4KiB: 9306.2 -> 983.7 B/op (-89.4%, prior-session baseline) CPU time also improved (record 1B: 220.29 -> 55.57 us/op), consistent with reduced GC pressure. This eliminates what was, on Apple/Android PALs, effectively the dominant per-op allocation source for steady-state TLS record read/write - the unpooled ciphertext buffer scales directly with record/payload size, so the win grows with payload size. Added a differential regression test (SslStream_StreamToStream_RepeatedVariableSizeWrites_RoundTripsCorrectly) exercising many back-to-back writes of varying sizes - including sizes that force ProtocolToken buffer growth via EnsureAvailableSpace - to guard against data corruption from pooled-buffer reuse across the Async/BeginEnd/Sync/Memory SslStream API surfaces. Full System.Net.Security suites pass with zero regressions: functional: 4977/0 failed, 19 skipped (4972 baseline + 5 new) unit: 132/0 failed, 3 skipped (unchanged) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e001a1f-f29f-4089-9224-6c63a5549120
ce59239 to
9828050
Compare
| ProtocolToken token = default; | ||
| token.RentBuffer = true; |
There was a problem hiding this comment.
Good suggestion, but deferring for now to keep this PR's diff minimal and easy to review/revert (it's already a 2-line, 2-file fix mirroring the existing Windows/OpenSSL pattern). A shared ProtocolToken pooled-init factory would be a good separate follow-up refactor across all 4 PALs, but I'd rather not fold a cross-cutting refactor into a targeted perf fix. Filing this as a mental note for a future PR rather than blocking this one on it.
…ostic Address automated review feedback: the comment referenced ProtocolToken.EnsureAvailableSpace, but the Apple/Android PAL code path actually exercised by this test goes through SafeDeleteSslContext.ReadPendingWrites + ProtocolToken.SetPayload, not EnsureAvailableSpace. Reworded to describe the test's intent (exercising pooled payload buffers of different rented sizes) without naming a specific internal method, so the comment stays accurate regardless of which PAL code path is taken. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e001a1f-f29f-4089-9224-6c63a5549120
|
@EgorBot -amd -osx_arm64 using System.Net;
using System.Net.Security;
using System.Net.Sockets;
using System.Security.Cryptography;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using BenchmarkDotNet.Attributes;
namespace SslStreamPoolingBenchmarks;
[MemoryDiagnoser]
public class SslStreamPoolingBench
{
private X509Certificate2 _serverCert = null!;
private X509Certificate2 _clientCert = null!;
private byte[] _payload1B = new byte[1];
private byte[] _payload1KiB = new byte[1024];
private byte[] _payload16KiB = new byte[16 * 1024];
private byte[] _payload64KiB = new byte[64 * 1024];
private byte[] _payload4KiB = new byte[4 * 1024];
[GlobalSetup]
public void Setup()
{
_serverCert = CreateSelfSignedCert("CN=sslstream-bench-server");
_clientCert = CreateSelfSignedCert("CN=sslstream-bench-client");
Random.Shared.NextBytes(_payload1B);
Random.Shared.NextBytes(_payload1KiB);
Random.Shared.NextBytes(_payload16KiB);
Random.Shared.NextBytes(_payload64KiB);
Random.Shared.NextBytes(_payload4KiB);
}
[GlobalCleanup]
public void Cleanup()
{
_serverCert?.Dispose();
_clientCert?.Dispose();
}
private static X509Certificate2 CreateSelfSignedCert(string subject)
{
using RSA rsa = RSA.Create(2048);
var req = new CertificateRequest(subject, rsa, HashAlgorithmName.SHA256, RSAExtensions());
req.CertificateExtensions.Add(new X509BasicConstraintsExtension(false, false, 0, false));
req.CertificateExtensions.Add(new X509KeyUsageExtension(
X509KeyUsageFlags.DigitalSignature | X509KeyUsageFlags.KeyEncipherment, false));
X509Certificate2 cert = req.CreateSelfSigned(DateTimeOffset.UtcNow.AddDays(-1), DateTimeOffset.UtcNow.AddDays(30));
return X509CertificateLoader.LoadPkcs12(cert.Export(X509ContentType.Pfx), (string?)null,
X509KeyStorageFlags.Exportable);
}
private static System.Security.Cryptography.RSASignaturePadding RSAExtensions() =>
System.Security.Cryptography.RSASignaturePadding.Pkcs1;
private static (NetworkStream client, NetworkStream server, Socket cs, Socket ss) CreateConnectedSockets()
{
using var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
listener.Listen(1);
var clientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
var connectTask = clientSocket.ConnectAsync((IPEndPoint)listener.LocalEndPoint!);
Socket serverSocket = listener.Accept();
connectTask.GetAwaiter().GetResult();
return (new NetworkStream(clientSocket, true), new NetworkStream(serverSocket, true), clientSocket, serverSocket);
}
private async Task<(SslStream client, SslStream server)> HandshakeAsync(bool mutualAuth, SslProtocols protocols = SslProtocols.None)
{
var (clientNs, serverNs, _, _) = CreateConnectedSockets();
var client = new SslStream(clientNs, false, (s, c, ch, e) => true);
var server = new SslStream(serverNs, false, mutualAuth ? (s, c, ch, e) => true : null);
var serverOptions = new SslServerAuthenticationOptions
{
ServerCertificate = _serverCert,
ClientCertificateRequired = mutualAuth,
EnabledSslProtocols = protocols,
};
var clientOptions = new SslClientAuthenticationOptions
{
TargetHost = "sslstream-bench-server",
EnabledSslProtocols = protocols,
ClientCertificates = mutualAuth ? new X509CertificateCollection { _clientCert } : null,
};
Task serverTask = server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None);
Task clientTask = client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None);
await Task.WhenAll(serverTask, clientTask);
return (client, server);
}
[Benchmark]
public async Task Handshake()
{
var (client, server) = await HandshakeAsync(mutualAuth: false);
client.Dispose();
server.Dispose();
}
[Benchmark]
public async Task MutualHandshake()
{
var (client, server) = await HandshakeAsync(mutualAuth: true);
client.Dispose();
server.Dispose();
}
private async Task RecordRoundTripAsync(byte[] payload)
{
var (client, server) = await HandshakeAsync(mutualAuth: false);
try
{
byte[] recvBuf = new byte[payload.Length];
Task writeTask = client.WriteAsync(payload, 0, payload.Length);
int total = 0;
while (total < payload.Length)
{
total += await server.ReadAsync(recvBuf, total, payload.Length - total);
}
await writeTask;
}
finally
{
client.Dispose();
server.Dispose();
}
}
[Benchmark]
public Task Record_1B() => RecordRoundTripAsync(_payload1B);
[Benchmark]
public Task Record_1KiB() => RecordRoundTripAsync(_payload1KiB);
[Benchmark]
public Task Record_16KiB() => RecordRoundTripAsync(_payload16KiB);
[Benchmark]
public Task Record_64KiB() => RecordRoundTripAsync(_payload64KiB);
private async Task PooledConnectionAsync(int concurrency)
{
var tasks = new Task[concurrency];
for (int i = 0; i < concurrency; i++)
{
tasks[i] = RecordRoundTripAsync(_payload4KiB);
}
await Task.WhenAll(tasks);
}
[Benchmark]
public Task Pooled_C1_4KiB() => PooledConnectionAsync(1);
[Benchmark]
public Task Pooled_C16_4KiB() => PooledConnectionAsync(16);
[Benchmark]
public Task Pooled_C64_4KiB() => PooledConnectionAsync(64);
} |
…t scope - Replace Assert.True(VerifyOutput(...)) with Assert.Equal for better xUnit mismatch diagnostics; remove now-unused VerifyOutput helper. - Rescope the existing regression test's comment to the encrypted write path specifically (it doesn't independently exercise handshake pooling). - Add SslStream_RepeatedIndependentHandshakes_RoundTripCorrectlyAfterEach, which runs 8 independent handshakes back-to-back so rented handshake token buffers are returned/re-rented across connections, verifying each still round-trips application data correctly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e001a1f-f29f-4089-9224-6c63a5549120
|
Status: still a work in progress — not ready for review. Since the last status update:
Remaining before this could be considered for review:
Note This comment was prepared with AI assistance (GitHub Copilot CLI) under human supervision. |
| int[] sizes = { 1, 2, 7, 64, 255, 1024, 4096, 16 * 1024, 64 * 1024, 3, 1, 65 * 1024 }; | ||
| var rng = new Random(12345); | ||
|
|
||
| foreach (int size in sizes) | ||
| { |
| const int HandshakeCount = 8; | ||
| var rng = new Random(54321); | ||
|
|
||
| for (int i = 0; i < HandshakeCount; i++) | ||
| { |
| // Regression test for buffer-pool reuse on the encrypted write path | ||
| // (SslStreamPal.*.EncryptMessage -> ProtocolToken.SetPayload). |
| // Regression test for buffer-pool reuse on the handshake output path | ||
| // (SslStreamPal.*.HandshakeInternal -> ProtocolToken.SetPayload). Runs several |
| ProtocolToken token = default; | ||
| token.RentBuffer = true; |
|
@wfurt - what are your thoughts on the merits and / or timing? (I'm still working on the PR as a background task.) |
area co-maintainer here, I would be okay with merging this even this late in the cycle. The risk is low since the other PAL implementation already rent the buffer (so any bugs in the shared logic would have already surfaced). |
| // Regression test for buffer-pool reuse on the encrypted write path | ||
| // (SslStreamPal.*.EncryptMessage -> ProtocolToken.SetPayload). | ||
| // Exercises many back-to-back writes of varying sizes -- including sizes much larger | ||
| // than typical, to exercise pooled payload buffers of different rented sizes -- and | ||
| // verifies data integrity across pooled-buffer reuse, so any accidental data bleed | ||
| // between reused rented buffers, truncation, or double-return corruption would be caught. | ||
| [Fact] | ||
| public async Task SslStream_StreamToStream_RepeatedVariableSizeWrites_RoundTripsCorrectly() | ||
| { |
There was a problem hiding this comment.
I don't think either of the new tests adds any value. There are enough existing tests (thousands) that if the buffer reuse logic was flawed, it would surface as nondeterministic failures in random tests across whole test suite runs.
|
making platforms differences smaller is generally ok -> this is not big new rewrite, I see it more as consolidation. |
I am running an experiment - full analysis to follow. Contact @artl93
Motivation
ProtocolTokenalready supports pooled (ArrayPool<byte>.Shared) payload buffers via aRentBufferflag, consumed correctly byReleasePayload(). The Windows (SChannel) and OpenSSL/Unix (Linux)SslStreamPALs already set this flag. The Apple (Secure Transport: macOS/iOS/tvOS/Mac Catalyst) and Android PALs never did — every encrypted handshake/write token on those two PALs was allocated fresh instead of rented, for no apparent functional reason (confirmed viagit blame/history:RentBuffersimply wasn't threaded through when those two PALs were added,#87874per @bartonjs; @wfurt confirmed these PALs "were never in center of attention" and flagged buffer-lifetime/leak risk as the reason to be cautious about rental in general).Change
Two lines, two files — set the existing
RentBuffer = trueflag at the two token-allocation call sites in:src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cssrc/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.csNo new code path is introduced —
ProtocolToken.ReleasePayload()already gates pool-return on this per-token flag and is already exercised by the Windows/OpenSSL PALs in production today. This only activates a pre-existing, already-tested pattern for the two PALs that were missing it.Evidence
Exact-parent A/B (this session — fully-matched Release config, both host/libs/runtime)
c1b09d933f7d720f7ae50f80b4ec6980bb93d96f982805085b2b7225b2fe5ff113e0a6a65d5084e611.0.100-preview.6.26352.110SDK,11.0.0runtime, both sides built with identical./build.sh clr+libs -rc release -lc release -hc release(this corrects an earlier internal pass that mixed a Debug testhost/host config with Release libraries — matching all three configs removes that variable)git worktreeper commit, isolated.dotnet/artifacts; a self-contained console harness executed via<testhost>/dotnet execagainst each side's own testhost (swaps the shared-frameworkSystem.Net.Security.dllcleanly — confirmed different SHA-256 hashes,2afb0fb4...candidate vs0bfb9c8d...baseline); 3 process launches per side, 5 in-process warm-up iterations + 10 measured trials each = n=30 trials/scenario/side, baseline/candidate runs alternated per round; allocation measured viaGC.GetTotalAllocatedBytes(precise: true)(process-wide —GetAllocatedBytesForCurrentThreadis unreliable acrossawaitcontinuations that resume on a different thread-pool thread, confirmed by a discarded first attempt yielding nonsensical negative deltas)† Time deltas of a few percent at these absolute magnitudes (TCP-loopback + RSA/AES/SHA-dominated) are within run-to-run noise for a whole-connection benchmark; this change targets allocations, not raw handshake/record latency. Allocation reduction is the reproducible signal, scales with record size as expected (bigger records ⇒ bigger rented buffers ⇒ proportionally larger savings), and the pooled/concurrent scenarios (representative of a reused,
HttpClient-style connection) show a consistent ~10% reduction.These end-to-end whole-connection percentages are intentionally more conservative than an earlier isolated-component pass (which measured the touched allocation call sites directly and saw −69%/−97%/−98% at 1 KiB/16 KiB/64 KiB, and ~−89% pooled). Both are correct for what they measure: the isolated number answers "how much did the touched code improve," the end-to-end number above answers "how much does a full connection improve" once diluted by everything else a handshake/record round-trip allocates (sockets, the
SslStreamobject graph, cert parsing, TLS state machine, task continuations). Both point the same direction and are mutually consistent.EgorBot
Requested (
-amd -osx_arm64) in a follow-up comment with a self-contained BenchmarkDotNet harness covering the same 9 scenarios (Handshake, MutualHandshake, Record_1B/1KiB/16KiB/64KiB, Pooled_C1/C16/C64_4KiB). Results pending — will be posted to this PR by the bot once its run completes.Validation
System.Net.Security.Tests, candidate HEAD): 4978 total, 0 failed, 20 skipped (macOS: TLS 1.3/renegotiation/null-encryption tests correctly skip — platform doesn't support those, unrelated to this change).System.Net.Security.Unit.Tests): 132 total, 0 failed, 3 skipped.SslStreamStreamToStreamTest.SslStream_StreamToStream_RepeatedVariableSizeWrites_RoundTripsCorrectly) exercising many back-to-back writes across a range of payload sizes — including large payloads that force different rented-buffer sizes — verifying data integrity across pooled-buffer reuse, so accidental data bleed between reused buffers, truncation, or double-return corruption would be caught.Risks / limitations
RentBuffer/ReleasePayload()mechanism rather than introducing new pooling logic — the same code path Windows/OpenSSL PALs rely on today. No new disposal/ownership contract is introduced.ArrayPoolconsumer, if a caller ever accessed aProtocolToken's buffer past itsReleasePayload()call, that would now retrieve pool-recycled memory rather than a discarded array — this pattern is already relied upon by the other two PALs and the test suite exercises repeated encrypt/decrypt cycles, but this is the class of risk @wfurt flagged and worth explicit reviewer attention.Note
This PR description, evidence, and validation were prepared with AI assistance (GitHub Copilot CLI) under human supervision. Still a work in progress — not yet ready for review (see final PR comment for current status).