perf(ext/node): remove per-chunk copying on TLSWrap data paths#35705
Conversation
Three behavior-preserving changes that cut allocation and copying on the TLS hot paths: - Deliver decrypted reads to JS via a backing store built from the bytes (one memcpy, zero-copy for the JS-stream enc-out path) instead of zero-initializing an ArrayBuffer and then writing it byte-by-byte through Cell accessors — two full passes over every decrypted chunk. - Serialize rustls output directly into pending_enc_out (Vec's io::Write impl appends) instead of round-tripping every record batch through a temporary 16KB Vec. - Track consumed pending_cleartext with an offset instead of re-allocating and copying the unwritten tail after every 48KB chunk, which made large writes O(n^2): a single 32MB socket.write() drops from ~760ms to ~50ms in a local loopback echo benchmark. Also removes the write-only fields EncryptedWriteReq.has_write_callback and TLSWrapInner.session_was_set, and adds unit tests covering enc_out collection and clear_in offset bookkeeping against a real rustls client connection. Co-authored-by: Cursor <[email protected]>
In workspace-wide CI builds, cargo feature unification enables both of rustls' crypto backends (ring and aws-lc-rs), so ClientConfig::builder can no longer infer the process-level provider and panics. Install the aws-lc-rs provider explicitly in the test helper, matching what ext/fetch's tests already do. Co-authored-by: Cursor <[email protected]>
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
One small suggestion on the offset bookkeeping — otherwise this looks great.
|
Took a close look at the buffer handling here (offset invariants, the JS handoff paths, and the removed fields) and this is almost ready to go. The offset bookkeeping in
The verification section is unusually thorough (byte-identical node_compat failure set vs a main baseline, hash-verified 32 MB transfers). Nice work. |
|
Thanks, included the feedback now |
|
Verified in a release build with a clean same-tree A/B: both binaries are release builds of this branch's tip (
The accidental-quadratic is clearly visible: the baseline grows ~3x per doubling (O(n²)), while this PR grows ~2x per doubling (O(n)), so the win scales with payload size. At the 32 MiB point from the description that's 11.6x in release (276 ms -> 24 ms); the description's 760 ms -> 50 ms was a debug build, where the per-byte memcpy cost is higher, so the absolute numbers differ but the fix reproduces and is even more dramatic at 64 MiB (~20x). Integrity check passed on every run (byte-exact receipt). Nice fix. ✅ |
3 participants
A single 32 MB
socket.write()through anode:tlsloopback connection drops from ~760 ms to ~50 ms (debug build, macOS, 3 runs each) with this change. The cause was accidentally-quadratic buffer management inclear_in(): cleartext is fed to rustls in 48 KB chunks, and after every chunk the entire unwritten tail was re-allocated and copied (data[offset..].to_vec()). For an n-byte write that's ~n²/96KB bytes of cumulative memcpy — about 1 GB of copying for a 10 MB write.This PR fixes that plus two smaller per-chunk costs on the same data paths. All three changes are behavior-preserving:
clear_in()tail copying: track consumed bytes with apending_cleartext_offsetinstead of re-allocating the remainder after every chunk. The buffer is released once fully consumed, and the write-error semantics (drop the tail) are unchanged.do_emit_read): build theArrayBufferfrom a backing store created from the bytes (one memcpy) instead ofArrayBuffer::new— which zero-initializes the allocation — followed by a byte-by-byte copy throughCellaccessors. That was two full passes over every decrypted chunk handed to JS. Same pattern already used inext/node/ops/http2/session.rsandllhttp/binding.rs.enc_out_collect/enc_out_flush_only):write_tlsserializes directly intopending_enc_out(Vec'sio::Writeimpl appends) instead of round-tripping every record batch through a temporary 16 KBVec.Also removes two write-only fields found while auditing these paths (
EncryptedWriteReq.has_write_callback,TLSWrapInner.session_was_set—isSessionReused()reports from the negotiated handshake kind, sosetSessionis now an explicit no-op), and adds two unit tests against a real rustlsClientConnection: one coveringenc_out_collectgathering/non-duplication, one coveringclear_in's offset bookkeeping (bounded chunks, monotonic progress, buffer never mutated while partially consumed, released when drained).Verification
cargo test -p deno_node --lib tls_wrap— 5/5 (includes the 2 new tests)unit_node::tls_test,unit_node::https_test,unit_node::http2_test— passspecs::node::tls_*spec tests — passmainbaseline build run on the same machine (57 pre-existing failures, no additions/removals)tools/format.jsandtools/lint.jspassI used Cursor (Claude) to help investigate and write this change.