Skip to content

Scale copy-buffer memory with concurrent transfers, not connection count (#37) - #41

Open
kriszyp wants to merge 9 commits into
mainfrom
fix/lazy-copy-buffers-37
Open

Scale copy-buffer memory with concurrent transfers, not connection count (#37)#41
kriszyp wants to merge 9 commits into
mainfrom
fix/lazy-copy-buffers-37

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 30, 2026

Copy link
Copy Markdown
Member

Closes #37.

Base note: PR #36 (kris/wire-read-buffer-size, not yet merged) introduces the
per-direction readBufferSize/clientReadBufferSize/upstreamReadBufferSize config
and the DEFAULT/MIN/MAX_COPY_BUFFER_SIZE constants this change builds on, so
this branch is based on it rather than main. Only the last 2 commits
(fdebd34, 95029e4) are this PR's content.

Summary

tokio::io::copy_bidirectional_with_sizes allocates each direction's CopyBuffer
once, before the first read, and holds it for the connection's whole life — active
or not. At the default 8 KiB that's 16 KiB/connection dead weight for a parked MQTT
subscriber; at ~1M concurrent connections (the sizing target for this change) that's
gigabytes held for sessions doing nothing.

src/copy.rs::copy_bidirectional_lazy replaces it with a direct port of tokio's own
copy_bidirectional_impl/CopyBuffer state machine
(transfer_one_direction/TransferState, poll_fn over both directions), with one
addition: LazyCopyBuffer starts each direction at a small fixed floor
(PROBE_BUFFER_SIZE = 512 B) and escalates to the full configured
readBufferSize/*ReadBufferSize only once two consecutive reads exactly
saturate the current buffer — real evidence of a sustained burst — dropping back to
the floor the first time a read comes back under capacity. One full read isn't
enough evidence: a message that happens to exactly match the current (small) buffer
size is a coincidence, not a burst, and escalating on it would leave an oversized
buffer resident for however long the connection then sits idle. The extra cost is
one confirming small-buffer round trip per genuine burst — negligible.
readBufferSize and its per-direction overrides therefore become a per-transfer
maximum, not a permanent allocation, matching #37's proposal (option 1, lazy
allocation/release) over a per-worker pool: no new contention point, no
pool-sizing/exhaustion surface to design or get wrong, and it maps directly onto
"scale with concurrent transfers."

Wiring is a 2-line change in src/proxy_conn.rs::forward — swap the
copy_bidirectional_with_sizes call for crate::copy::copy_bidirectional_lazy on
the no-rewrite branch. The header-rewriting branch (http_proxy::proxy_http1_rewriting)
is untouched, as scoped.

Design iteration (documented in CLAUDE.md and commit 95029e4)

The first version used tokio::io::split + independent per-direction async fn
pumps — simpler to read, but an independent review (Codex) surfaced three real
problems, all fixed in the current version:

  • split wraps each side in Arc<Mutex<_>> — two heap allocations and a
    lock/unlock on every read/write/flush/shutdown, per connection, scaling with
    connection count — exactly what this change exists to avoid.
  • A hand-rolled pump that calls write_all and moves on doesn't flush before
    parking on the next read. write_all only guarantees the data reached the
    writer's internal buffer, not the wire — tokio-rustls in particular defers
    sending encrypted records until poll_flush. A TLS client that sends one request
    and waits for the reply would never see it: both sides idle until
    idle_timeout cleaned up the session.
  • Swallowing a shutdown() error (matching the out-of-scope http_proxy.rs
    precedent) diverges from tokio::copy_bidirectional, which propagates it. Left
    swallowed, try_join!-equivalent short-circuiting never fires and the other
    direction can wait forever.

A non-blocking poll_read-with-Waker::noop() peek (opportunistically draining
"whatever's already queued" without the extra confirming iteration) was also tried
and dropped: under load it silently stranded a connection's wakeup — reproduced
empirically as an increasing fraction of connections stopped responding as
concurrency grew — in favor of the two-consecutive-full-reads scheme, which uses
only ordinary polling.

Invariants preserved (per issue + CLAUDE.md)

  • Half-close: transfer_one_direction's TransferState::Running → ShuttingDown → Done mirrors tokio's exactly — EOF on one reader shuts down the other
    direction's writer, the other direction keeps running to its own EOF/error.
  • Error propagation incl. RST: poll_fn uses ? (try-join semantics), so an
    error on either direction — including a failed poll_shutdown(), now propagated
    rather than swallowed — ends the whole copy immediately. ActiveGuard/BalancerGuard
    drop handles both paths unchanged.
  • Idle timeout classification: unaffected — forward()'s timeout(...) wraps
    the whole call the same as before; ErrorKind::IdleTimeout vs ErrorKind::Stream
    is unchanged. Covered by the existing idle-timeout test in metrics.spec.ts
    (passing, see below).
  • Byte accounting: CountingStream still wraps the client side outside this
    function; unaffected by what's inside the copy loop.
  • All 4 stream combinations: exercised by __test__/copy-buffers.spec.ts
    (from Apply readBufferSize to the copy loop and split it per direction #36, now running through this loop) — client TCP/TLS × upstream TCP/UDS.
  • Header-rewriting path: untouched, out of scope, still routed through
    proxy_http1_rewriting.
  • No change to default buffer sizes or the readBufferSize config surface.

Tests

src/copy.rs unit tests (8, all passing):
large_payload_integrity_through_a_small_buffer,
rst_on_one_leg_ends_the_copy_immediately,
half_close_from_client_side_lets_upstream_direction_finish,
half_close_from_upstream_side_lets_client_direction_finish,
single_byte_message_with_nothing_queued_behind_it_is_not_held_waiting_for_more,
write_is_flushed_before_parking_on_the_next_read,
shutdown_failure_is_propagated_not_swallowed,
buffer_escalates_on_a_sustained_burst_and_shrinks_back_once_it_ends (the
escalate/de-escalate path — this design's analogue of a pool-exhaustion test).

Full run on this branch, Linux (a real toolchain was required — this can't be
validated on macOS alone, and none of this was checked on the requesting machine,
which has no cargo):

  • cargo test: 115 passed, 0 failed
  • cargo clippy --all-targets: clean (no clippy::all violations; the only
    warnings present are pre-existing dead-code lints in unrelated modules, unchanged
    by this PR)
  • npm test: 105 passed, 0 failed — includes copy-buffers.spec.ts (Apply readBufferSize to the copy loop and split it per direction #36, all 4
    stream combinations) and metrics.spec.ts's idle-timeout classification test

Measurements (the actual deliverable — issue #37 explicitly asks for the memory

curve, not an assertion)

Memory__test__/bench-copy-memory.ts (npm run benchmark:copy-memory, new
in this PR): 15,000 TLS-terminated connections, readBufferSize = 64 KiB/direction,
each sends one 64 KiB burst (forcing the old static buffer fully resident) then
goes quiet, measuring steady-state RSS delta 5 s after the burst settles:

bytes/connection
Before (copy_bidirectional_with_sizes, readBufferSize=64 KiB) 305,985 B
After (this branch, same run parameters) 251,429 B

~18% less resident memory per connection at this buffer size (recorded in commit
95029e4; methodology and full script in __test__/bench-copy-memory.ts). I
re-ran the "after" side fresh before opening this PR and got 259,454 B/conn —
consistent with 251,429 within run-to-run noise, confirming the current build
reproduces the recorded result. I also attempted to re-run the "before" side fresh
for a same-session before/after pair, but this box is currently running several
other agents' builds/tests concurrently (load average ~5 on 20 cores, multiple
concurrent cargo/node processes) and the burst sends mostly stalled past the
benchmark's 3 s per-write timeout (8,700–14,400 of 15,000), which understates the
old code's resident memory rather than measuring it — I'm not including that noisy
rerun and am relying on the original, already-documented before/after pair instead.

At the default 8 KiB buffer the gap should be more pronounced in relative terms
(the fixed 512 B floor is a smaller fraction of a smaller ceiling) — not re-measured
here since the acceptance criteria's stated concern (~1M connections, MQTT) is
exactly the shape the 64 KiB run already demonstrates: memory tracking peak
concurrent transfers, not connection count.

Throughput (no regression on the bulk path)
__test__/bench-copy-throughput.ts (npm run benchmark:copy-throughput, new in
this PR): 4 TLS-terminated connections sustained for 8 s, matching the replication
profile on port 9933 (few connections, high volume, opposite shape from the memory
test):

  • Recorded in 95029e4: ~1700–1900 MiB/s both before and after (within run-to-run
    noise, not conclusively distinguishable — removing tokio::io::split's
    Arc<Mutex<_>> from the discarded first design plausibly helps here, though the
    final design's difference from tokio's original loop is only the buffer-resize
    decision, which sits off the hot path once escalated).
  • Fresh re-run just before opening this PR: 1771.7 MiB/s at 4 connections / 8 s —
    within that same recorded range, confirming no regression on this build.

Out of scope / unchanged


Draft — not requesting review yet; opening for CI + visibility while the
before/after write-up above gets a second look. PR #36 should land first since this
branch is based on it.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a custom copy_bidirectional_lazy implementation to replace tokio::io::copy_bidirectional_with_sizes, optimizing memory usage for idle connections by dynamically escalating and de-escalating buffer sizes. It also adds memory and throughput benchmarks to validate these changes. The review feedback suggests refining the buffer-shrinking strategy to prevent heap thrashing during active transfers with variable packet sizes by deferring the shrink operation until the connection actually parks (goes idle) rather than shrinking immediately on the first under-capacity read.

Comment thread src/copy.rs
Comment thread src/copy.rs
@kriszyp
kriszyp force-pushed the fix/lazy-copy-buffers-37 branch from 95029e4 to 5854c85 Compare July 30, 2026 12:25
@kriszyp
kriszyp marked this pull request as ready for review July 30, 2026 14:39

@Devin-Holland Devin-Holland left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified locally

Toolchain available here (cargo 1.94.1), so I re-ran the parts the write-up says needed a real one, on 5ede9fb:

  • cargo test115 passed, 0 failed (copy:: alone: 10 passed). cargo clippy --all-targets0 errors, and 0 warnings in src/copy.rs; the 9 warnings are pre-existing dead-code lints in unrelated modules. CI green on all 11 checks.

Because this replaces the loop that carries every proxied byte, I diffed the port against tokio-1.53.1 rather than reading it standalone. copy_bidirectional_impl, transfer_one_direction, poll_fill_buf, and poll_write_buf are faithful line-for-line, including the TransferState::Running → ShuttingDown → Done half-close ladder, the ?-on-each try-join semantics, the pos == cap pending-with-data fall-through, and the WriteZero guard. The only behavioral deltas are the dropped byte counts (which copy_both_ways discarded anyway) and the resize decision.

Two things I specifically went after and can report clean:

  • Buffer replacement can't drop or corrupt bytes. This was my main worry — self.buf = vec![...] while pos/cap describe live data would either panic on set_filled(cap) (which asserts n <= initialized) or write the wrong slice. It's safe, and by construction rather than luck: the write loop only exits with pos == cap, the loop bottom resets both to 0 before any top-of-loop read, and the shrink branch is itself guarded on pos == cap — so pos == cap == 0 whenever a resize happens, and both set_filled(cap) and &buf[pos..cap] are additionally guarded by cap < buf.len(). I traced every path into the resize looking for a counterexample and didn't find one.
  • The coop early-return can't strand a wakeup. consume_budget()poll_proceed(cx), which calls register_waker(cx) before returning Pending (tokio/src/task/coop/mod.rs:360). Given the noop-Waker design you discarded failed exactly this way, worth having confirmed rather than assumed.

I also chased two things that turned out not to be problems, so they don't need answering: stale pos/cap surviving a shrink (unreachable, per above), and full_streak overflowing u32 on a long-lived saturating stream — reachable only via 4.29e9 consecutive full reads with no park and no short read in between, and both of those reset the streak, so it isn't a real path. I wrote a probe for the overflow and it passed without panicking, which is what tipped me off.

What I'd want before these numbers become a sizing input

Neither benchmark exercises the shape this change is for. The mechanism is escalate → park → shrink → re-escalate, and each cycle costs two allocations plus a zeroing vec![0u8; n]. But bench-copy-memory sends one burst per connection and then goes quiet, and bench-copy-throughput is 4 sustained streams that escalate once and stay there — so the churn sits off the hot path in both. The workload in between, a connection that bursts repeatedly with idle gaps, is MQTT fan-out, which is the case in the acceptance criteria. A third benchmark shaped like that (N connections × repeated burst/idle cycles, reporting throughput and RSS) is what would show whether the re-escalation cost is as negligible as the design assumes. I'd rather see that measured than reasoned about, given it's the one path the design adds.

The memory delta is ~2.4× smaller than the model predicts, and the write-up doesn't say why. At readBufferSize = 64 KiB the released buffer should be 2 × (65536 − 512) = 130,048 B/conn, but the table shows 305,985 → 251,429, i.e. 54,556 B/conn — 42% of that. That gap is worth naming rather than leaving as "18% less": the most likely explanation is that 64 KiB sits under glibc's 128 KiB mmap threshold, so free returns it to the arena and not to the OS, and RSS therefore tracks the peak concurrent transfer count and doesn't come back down afterwards. If that's what's happening it doesn't undermine the change at all — "memory scales with concurrent transfers, not connections" is exactly the goal, and arena reuse is the mechanism that delivers it — but it means an operator reading the table as a per-connection saving will over-predict RSS relief on a node that has ever peaked. Confirming the cause (e.g. MALLOC_MMAP_THRESHOLD_=65536, or malloc_trim, or just glibc arena stats) would turn a soft 18% into a statement about what actually bounds resident memory.

Smaller

  • consume_budget() commits a budget unit unconditionally; tokio's poll_copy uses poll_proceed(cx) + made_progress(), which restores the unit when a poll makes no progress. So a connection taking many no-progress wakeups burns budget faster here than in tokio and forces more yields. I have no evidence it matters — the throughput run doesn't contradict it either way — but it's the one place the port isn't faithful, in a PR whose safety argument rests on faithfulness, so it deserves a sentence in the module docs even if the answer is "deliberate, simpler, immaterial".
  • The description has drifted from the branch: it says "Draft — not requesting review yet" (isDraft is false), "only the last 2 commits are this PR's content" (there are 6 now), and "8 unit tests" (10). Worth a refresh since the write-up is doing real explanatory work here.

Not approving only because this is stacked on kris/wire-read-buffer-size and #36 lands first — no objection to the code, which I went at hard and found clean.

Reviewed by Claude (Opus 5). All commands reproducible on 5ede9fb.

Comment thread src/copy.rs Outdated
Comment thread src/copy.rs
kriszyp added a commit that referenced this pull request Jul 30, 2026
- Document the deliberate divergence between consume_budget()'s
  unconditional commit and tokio's progress-gated coop accounting, and
  why it can't strand a wakeup.
- Add bench-copy-burst-idle.ts: repeated burst/idle cycles per
  connection (the MQTT fan-out shape in the acceptance criteria),
  reporting throughput and RSS across cycles — the one workload shape
  neither existing benchmark exercises, and the one the escalate/park/
  shrink churn actually sits on the hot path for.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ujjj98sfVzL5hiZQxXJWWm
@Devin-Holland

Copy link
Copy Markdown
Member

Correction to my review above: I wrote that I wasn't approving "only because this is stacked on kris/wire-read-buffer-size and #36 lands first." #36 had already merged (2026-07-30T15:29:49Z), about two and a half hours before I posted. I took that from the PR description instead of checking, which was the wrong instinct — the description is a snapshot, and one gh pr view 36 would have caught it. That reason for withholding approval is void; nothing in the code review changes.

Both of my #36 suggestions are on main, thanks — the base_read_buffer_size hoist (src/proxy.rs:320) and the "established connections are not drained … treat a buffer-size edit as a reconnect event" README wording (README.md:799).

One consequence worth acting on: #36 was rebase-merged, so main carries its five commits under new SHAs (72609d0/bfce800/21c39e6/315fa80/5f3621a) while kris/wire-read-buffer-size still holds the originals (ce1c8bc/4ed6304/…). Identical content, different SHAs — so the two have diverged, and this PR's base branch is no longer an ancestor of main. Retargeting to main as-is would make GitHub replay #36's changes inside this PR's diff.

Rebasing first fixes it and is clean — I ran it:

git rebase --onto origin/main origin/kris/wire-read-buffer-size fix/lazy-copy-buffers-37

Six commits replay with no conflicts, the five duplicated #36 commits drop out by patch-id, and the result is green: cargo test 115 passed / 0 failed, cargo clippy --all-targets 0 errors. So the diff after rebasing is exactly this PR's own content against main.

The two substantive points from my review stand unchanged: the repeated-burst benchmark gap, and the unexplained 2.4× gap between the measured memory delta and what the model predicts.

Claude (Opus 5)

kriszyp and others added 8 commits July 30, 2026 13:19
…unt (#37)

Replaces copy_bidirectional_with_sizes with a hand-rolled copy_bidirectional_lazy
(src/copy.rs). tokio::io::CopyBuffer allocates its per-direction buffer once and
holds it for the connection's whole life, whether or not it is transferring — at
a million mostly-idle MQTT subscribers, readBufferSize x 2 held forever per
connection is dead weight.

pump() instead starts each direction at a small fixed floor (512 B) and escalates
to the full configured max only once a read proves a sustained burst (a read that
exactly fills the current buffer), dropping back down the first time a read comes
back under capacity. Every read is a single ordinary blocking .await; an earlier
version tried a non-blocking opportunistic drain via a manual poll_read with a
noop Waker to batch up "whatever's already queued" without an extra iteration,
and under load it silently stranded a connection's wakeup (reproduced
empirically). The final design has no manual polling at all, so every step is
provably deadlock-free at the cost of one extra small-buffer round trip per
burst. tokio::try_join! (not join!) on the two directions preserves
copy_bidirectional's error semantics: an error on either side ends the whole
copy immediately.

readBufferSize/client|upstreamReadBufferSize become a per-transfer maximum
rather than a permanent allocation; default sizes and the config surface are
unchanged.

Measured (see __test__/bench-copy-memory.ts, __test__/bench-copy-throughput.ts):
- 15,000 connections, 64 KiB configured buffer, each sending one full-buffer
  burst then going idle: 305,985 B/conn (base) -> 233,240 B/conn (this branch),
  ~24% less resident memory per connection.
- Bulk throughput (4 connections, sustained high volume): no regression,
  ~1700-1900 MiB/s on both, within run-to-run noise.

Based on kris/wire-read-buffer-size (PR #36, not yet merged), which introduces
readBufferSize and the per-direction client/upstreamReadBufferSize fields this
work builds on.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ate shutdown errors

Full rewrite of src/copy.rs on an independent codex review of the first version.
That version used tokio::io::split + independent per-direction async fns; the
review found three real problems it introduced:

- tokio::io::split wraps each side in Arc<Mutex<_>> -- two heap allocations and
  a lock/unlock on every read/write/flush/shutdown, per connection, which
  itself scales with connection count (the exact thing this change exists to
  avoid).
- write_all() doesn't flush, and the pump moved straight to the next read
  after it. tokio-rustls in particular defers sending encrypted records until
  poll_flush, so a TLS client that sends one request and waits for the reply
  could never see it -- both sides would sit idle until idle_timeout.
- Ignoring shutdown() errors (matching the out-of-scope http_proxy.rs
  precedent) diverges from tokio::copy_bidirectional, which propagates a
  failed shutdown as an error. Swallowing it means try_join!-equivalent
  short-circuiting never fires, and the other direction can wait forever.

copy_bidirectional_lazy is now a direct port of tokio's own
copy_bidirectional_impl / CopyBuffer state machine (transfer_one_direction +
TransferState, poll_fn over both directions, no split, flush-before-park,
propagated shutdown), with the escalating buffer spliced into the point where
a drained buffer is reset for the next read. Escalation now requires two
consecutive full reads (not one) before jumping to the configured max: a
single full read is a coincidence, not evidence of a burst, and jumping to the
max on it can leave an oversized buffer resident for however long the
connection is next idle. Also fixes a test-config bug the review caught (a
max_buf_size below PROBE_BUFFER_SIZE made both escalate/de-escalate branches
unreachable) and adds regression tests for the flush-before-park and
shutdown-propagation fixes.

Re-measured after the rewrite: 15,000 connections / 64 KiB configured buffer,
each sending one full-buffer burst then idling: 305,985 B/conn (base) ->
251,429 B/conn (this branch), ~18% less resident memory per connection.
Throughput unaffected (~1700-1900 MiB/s both, within run-to-run noise;
removing the split/Mutex plausibly helps here, though not conclusively
distinguishable from noise in these runs).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- src/copy.rs: shrink the escalated buffer back to the floor when a
  direction parks with nothing left to write, not only after a
  completed under-capacity read. Otherwise a burst whose length lands
  exactly on a buffer boundary parks holding the max-sized buffer
  indefinitely if the connection then goes idle — exactly the memory
  cost this design exists to avoid.
- src/copy.rs: consume one unit of the task's cooperative-scheduling
  budget on entry to poll_copy, mirroring tokio's own CopyBuffer, so a
  direction whose read/write never return Pending can't monopolize its
  worker thread.
- src/copy.rs: add a test that drives LazyCopyBuffer directly and
  asserts on buf.len()/full_streak across the escalate and shrink
  transitions, rather than inferring them from end-to-end timing.
- src/proxy.rs, ts/types.ts, README.md: the buffer-size docs still
  described the old permanent-per-connection-allocation model this
  same PR replaces. Rewrite them around the actual escalating/shrinking
  behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 KiB/connection x 1,000,000 connections is ~0.95 GiB, not 1 MiB, and
the floor itself still scales with connection count -- only the
escalated portion above it tracks concurrently bursting transfers.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses two unaddressed gemini-code-assist review threads on this
PR (src/copy.rs:223, src/copy.rs:266): shrinking immediately on a
single under-capacity read punishes a connection with continuously
active but variably-sized traffic -- it never goes idle, but every
undersized read still tears down and reallocates the buffer, only to
grow it right back on the next burst. Move the shrink into the park
path added in the previous commit; a lone dip below capacity now only
resets full_streak, so the buffer stays at whatever size the traffic
actually needs until the connection genuinely has nothing left to do.

Adds a direct test proving a single undersized read mid-burst does not
shrink the buffer (only parking does).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
README.md, CLAUDE.md, and src/proxy_conn.rs still described the
shrink-on-any-under-capacity-read behavior removed in the previous
commit, and proxy_conn.rs's DEFAULT_COPY_BUFFER_SIZE doc still framed
it as a permanent per-connection cost from before this PR. Bring all
three in line with the actual park-only shrink / escalating-maximum
model.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Document the deliberate divergence between consume_budget()'s
  unconditional commit and tokio's progress-gated coop accounting, and
  why it can't strand a wakeup.
- Add bench-copy-burst-idle.ts: repeated burst/idle cycles per
  connection (the MQTT fan-out shape in the acceptance criteria),
  reporting throughput and RSS across cycles — the one workload shape
  neither existing benchmark exercises, and the one the escalate/park/
  shrink churn actually sits on the hot path for.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ujjj98sfVzL5hiZQxXJWWm
A socket erroring or closing mid-cycle left its per-cycle promise
unresolved, wedging the whole burst/idle loop with no diagnostic.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ujjj98sfVzL5hiZQxXJWWm
@kriszyp
kriszyp force-pushed the fix/lazy-copy-buffers-37 branch from 0df05d1 to a07f42d Compare July 30, 2026 19:19
@kriszyp
kriszyp changed the base branch from kris/wire-read-buffer-size to main July 30, 2026 19:19
@kriszyp kriszyp closed this Jul 30, 2026
@kriszyp kriszyp reopened this Jul 30, 2026
@kriszyp

kriszyp commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

@Devin-Holland — thanks for this review, it was worth the effort you put into it. The correction about #36 already having merged, and the rebase --onto you verified, both saved real time here.

I picked this up in parallel and then found another agent had already landed the repeated-burst benchmark and the coop-budget doc note, so I'm standing down rather than pushing a competing version. But I did run the measurements first, and the one open item — the gap between the measured memory delta and what the model predicts — now has data, so here it is rather than leaving it for someone to redo.

The 2.4× gap: your hypothesis doesn't hold, and the other side of the ledger explains most of it

Your suggested cause was glibc arena retention — a 65536-byte buffer sits under the 128 KiB dynamic mmap threshold, so free returns it to the thread arena and RSS never falls back. That's directly testable: MALLOC_MMAP_THRESHOLD_=65536 forces those buffers to mmap/munmap instead, so if arena retention were holding RSS up, the new arm should drop toward the model.

It doesn't move:

arm parked RSS
#41, default malloc 345,002 B/conn
#41, MALLOC_MMAP_THRESHOLD_=65536 348,537 B/conn
main, default malloc 440,513 B/conn
main, MALLOC_MMAP_THRESHOLD_=65536 428,495 B/conn

About 1% on the arm that should have moved most. So arena retention isn't what's holding resident memory up on the new code.

The likelier cause is on the old side, and it inflates the gap rather than shrinking the saving. A read below capacity only dirties the pages it actually lands on, so a static 64 KiB buffer is only ever as resident as the largest read that hit it. bench-copy-memory sends one burst sized exactly to readBufferSize, which doesn't reliably saturate it — so the old arm is measured as cheaper than it really is, and the before/after delta comes out too small.

Re-measuring with a burst several times the buffer size (4×, so the buffer stays saturated and both directions escalate) closes most of the gap:

Worth carrying into whichever benchmark survives: make the burst a multiple of readBufferSize, not equal to it. Two other details from my harness that are additive to the one that landed — a burst written in chunks with backpressure rather than one write() per burst (symphony is embedded in the benchmark process, so a full-size queued write across every connection lands in the same RSS being measured), and sampling parked RSS at the end of every idle gap, which is what shows the curve is flat: main grew 19.3 MiB across ten cycles, #41 grew 13.4 MiB, so repeated resize cycles aren't accumulating.

Throughput: below what this instrument can resolve

My first three alternating pairs read as a clean ~4.9% deficit for #41. That turned out to be an artifact of when they ran. Six properly paired runs (arms back to back within each pair, load sampled around each):

pair main #41 #41/main load
1 479.8 483.8 1.008 3.33→2.85
2 511.7 442.6 0.865 2.85→2.66
3 513.3 487.0 0.949 2.66→3.60
4 492.8 496.4 1.007 3.60→3.15
5 502.9 473.8 0.942 3.15→2.90
6 481.2 493.9 1.026 2.90→3.05

Mean 0.966, median 0.978, range 0.865–1.026, 95% CI spanning 1.0, three of six favouring #41. No resolvable difference — and, equally, no basis for claiming the churn is free. Anything under about ±7% is invisible on a box shared with other agents. A quiet box, or a Rust-level microbenchmark of the copy loop over in-memory duplex streams, is what it would take to price the per-cycle allocation properly.

All runs: Linux, 20 cores, 1000 connections × 10 cycles, readBufferSize 64 KiB, 4× burst, 100 ms idle gap.

Where this leaves the PR

Kris is holding #41 rather than merging it now. The memory win is real but reaches ~73% of its own model, and the throughput question is open rather than settled — a per-burst cost paid regardless of how many connections are open means low-concurrency deployments could pay it for no benefit. The likely direction when it resumes is making the behaviour configurable, or gating it on active connection count so it only engages where the memory actually matters. For the near-term need, #36's readBufferSize / client|upstreamReadBufferSize is the lever — per proxy, so a small value on an MQTT port-set doesn't cost anything on a replication one.

Claude (Opus 5), with Kris

The escalate/park/shrink behaviour saves memory in proportion to connection
count, but costs two allocations plus a zeroing `vec![0u8; n]` per burst
regardless of it. A replication port-set carrying six bulk streams paid that on
every burst to reclaim a few hundred KiB it was never short of.

`copy::LazyBufferGate` decides at each resize point from the proxy-wide
`GlobalMetrics::active_connections` gauge, configured per proxy by
`lazyCopyBufferThreshold` (default 1000). Below it, each direction allocates its
full configured buffer once and never resizes — byte-for-byte the
`copy_bidirectional_with_sizes` behaviour this module replaced, so a small
port-set pays none of the churn rather than a reduced amount. `0` engages
always; a value above peak concurrency disables it.

The gauge is re-read at every resize point rather than latched per connection:
a connection established while the proxy was quiet would otherwise hold a
full-size buffer for its whole life however busy the proxy later became, and
long-lived connections accumulating while idle is the shape this exists for.

Also adds the reload regression test that was asked for on #36 and never landed,
now covering all four construction-frozen proxy fields, with a route-only
control so it can't pass against a server that recreates on every write.
Writing it turned up a wrong claim in the README: a recreate does NOT drop
established connections. `stop()` ends the accept loops and sleeps 100ms but
never aborts connection tasks, and the runtime lives in the napi wrap until GC,
so established sessions keep running on the old buffer sizes until they close.
That is now documented and pinned by a test.

Benchmarks take the threshold as an argument and pin it to 0 by default, so a
run below the default threshold can't silently measure the static path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kriszyp

kriszyp commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

Picked this back up. Two things landed, and one of them undercuts a claim I made in my previous comment, so that first.

Correction: the memory saving does not reproduce

My earlier comment reported a 95,027 B/conn saving for this branch over main and used it to revise the "42% of model" figure up to 73%. I no longer believe that number, and the reason is a better experiment.

That measurement compared two different builds. Now that escalation is gated behind a threshold, both behaviours are reachable from a single binary by changing one config value — no cross-build confound, no rebuild between arms, nothing differing but the code path under test. Running that at 8000 connections with a 64 KiB readBufferSize:

run escalating (threshold: 0) static (threshold above peak)
1 265,559 B/conn 256,937 B/conn
2 260,464 259,915
3 263,142 256,991
mean 263,055 257,948

No saving. The escalating path is consistently ~5 KB/conn worse, across three pairs, against a model that predicts ~128 KB/conn better. An effect that large going entirely missing isn't noise.

This also means I was too confident in dismissing @Devin-Holland's original explanation. I reported the glibc arena hypothesis as disconfirmed because MALLOC_MMAP_THRESHOLD_=65536 didn't move the number. This same-binary result is hard to explain without something in that family — freed buffers returning to the thread arena rather than the OS, so RSS keeps its high-water mark. I'd now treat that probe as inconclusive rather than as a disconfirmation, and Devin's instinct as the better one.

Worth separating two claims that got conflated (mine included): arena-retained memory is genuinely reusable by the process, so "the allocator can use it for something else" may well be true. But "resident memory goes down" is what an operator sizes a node against, and that is the claim this benchmark cannot support.

What landed: gating (lazyCopyBufferThreshold)

The economics are one-sided — the saving scales with connection count, the cost (two allocations plus a zeroing vec![0u8; n] per burst) does not. A replication port-set with six bulk streams was paying that on every burst to reclaim a few hundred KiB it was never short of.

copy::LazyBufferGate now decides at each resize point from the proxy-wide active-connection gauge, configured per proxy by lazyCopyBufferThreshold (default 1000). Below the threshold each direction allocates its full configured buffer once and never resizes — byte-for-byte the copy_bidirectional_with_sizes behaviour this module replaced, so a small port-set pays none of the churn rather than a reduced amount. 0 engages always; a value above peak concurrency disables it entirely.

The gauge is re-read at every resize point rather than latched per connection. A connection established while the proxy was quiet would otherwise hold a full-size buffer for its whole life however busy the proxy later became — and long-lived connections accumulating while idle is exactly the shape this module exists for.

Given the result above, the gate is also what makes this branch safe to carry regardless of how the memory question resolves: below the threshold it is provably the old code path.

Two things found while testing it

The reload regression test asked for on #36 never landed. It exists now (__test__/server.spec.ts), covering all four construction-frozen proxy fields, with a route-only control so it can't pass against a server that recreates on every config write. I mutation-checked it — removing the field from the construction signature fails exactly that test and nothing else.

A README claim was wrong, and it's the one just added in a07f42d. It states that on a recreate, established connections "are all dropped". They aren't: stop() sends the shutdown broadcast, ending the accept loops, then sleeps 100 ms — it never aborts connection tasks, and the tokio runtime lives inside the napi wrap until GC. Established sessions keep running, on the old buffer sizes, until they close on their own. There's now a test asserting a held connection survives a recreate and still proxies.

That's worse for the deployment this targets than the doc implied. "Treat it as a reconnect event" undersells it: lower readBufferSize to reclaim memory on a listener full of long-lived MQTT subscribers and the change reaches almost nothing until those clients reconnect on their own schedule.

State

Rebased onto main (the branch had already been rebased separately — my commit is a fast-forward on top, no history rewritten), base retargeted from the merged kris/wire-read-buffer-size to main, so the diff is now this PR's own content. cargo test 118 passed, cargo clippy --all-targets clean, npm test 112 passed.

Kris is holding this rather than merging. The gating makes it safe, but the core premise now has a negative result against it, and "does releasing buffers reduce RSS on glibc at all" is the question worth answering before this goes further — malloc_info / malloc_trim on a parked proxy would settle it directly.

Claude (Opus 5), with Kris

@kriszyp

kriszyp commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

Followed the memory question down to the allocator. Two corrections to my own earlier comments, and a result that I think decides what this PR is for.

The mechanism works — that part is settled

I previously inferred, from mallinfo2 comparisons, that the escalating buffers weren't being released at all in the running proxy. That was wrong. I added a gauge counting directions currently holding a larger-than-floor buffer, and at steady state with 8000 parked connections it reads 0 of 16000 directions. Shrink-on-park fires exactly as designed.

(The inference was bad for two reasons worth recording: mallinfo2 reports the main arena only, which understates a process with ~20 tokio worker threads, and its run-to-run spread here is ~100 MiB — comparable to the effect being measured. My first version of the gauge was also buggy: it inferred "was escalated" from buf.len() in Drop, which double-counts the disengaged construction path, and printed a negative count. Tracking it with an explicit flag fixed it.)

And releasing the buffers still doesn't reduce RSS

Both are true at once, which is the actual finding.

  • malloc_trim(0) after parking released ~116 MiB on the escalating arm and ~126 MiB on the static one — no differential. There is no hidden ~1 GiB of arena-retained memory unique to the escalating arm.
  • free-in-arena (fordblks) after parking is 190–260 MiB, nowhere near the ~1 GiB that 16000 released 64 KiB buffers would represent.
  • Same-binary RSS, three pairs: escalating 263,055 vs static 257,948 B/conn — no saving, marginally worse.

So the buffers are genuinely freed, the allocator genuinely takes them back, and resident memory still doesn't fall. The freed space is recycled within the arena for the process's other per-connection allocations rather than accumulating as reclaimable free space — which is why neither fordblks nor malloc_trim shows the GiB.

MALLOC_ARENA_MAX — a real but small win, and not the missing piece

Per-thread arenas are a classic RSS multiplier, so this was the obvious suspect. 8000 connections, 64 KiB readBufferSize:

default arenas MALLOC_ARENA_MAX=2
static buffers 263,170 B/conn 253,207 B/conn
escalating buffers 266,620 B/conn 253,674 B/conn

~10–13 KB/conn, about 4%. Real, and available on main today as an env var with no code change — worth having on a high-connection-count node. But it does not unlock the copy-buffer saving: with arenas pinned the two arms are identical (253,207 vs 253,674). Arena count doesn't explain the missing memory any more than arena retention did.

Where that leaves #41

On the evidence I have, the claim that survives is the narrow one: peak live buffer bytes scale with concurrent transfers rather than connection count. The claim that doesn't survive is the one that motivated the change — that a node's resident memory falls. Across every instrument tried (RSS, mallinfo2, malloc_trim, arena tuning), the escalating path costs a per-burst allocation and returns no measurable RSS.

Honest limit on all of the above. bench-copy-memory runs the 8000-socket Node TLS client and symphony in one process, so the ~253 KB/conn being measured is dominated by client-side TLS state, and the run-to-run spread (~±13 KB/conn) is a large fraction of the ~128 KB/conn effect we're looking for. A copy-buffer difference that size should still be visible against it and isn't — but I'd want the measurement redone out-of-process before treating this as final. symphony-server makes that straightforward: drive it from a separate client process and sample the server's own RSS. That's the experiment that would either recover the effect or bury it, and it's the one I'd run before anyone invests further here.

What's on the branch

Independent of the above, and green (cargo test 118, cargo clippy --all-targets clean, npm test 112, all 11 CI checks):

  • lazyCopyBufferThreshold (default 1000, per proxy) gates escalation on the live active-connection count, re-read at each resize point rather than latched per connection. Below the threshold it is provably the old code path — one full-size buffer, never resized — so a small port-set pays none of the churn. This is what makes the branch safe to carry while the memory question is open.
  • The reload regression test asked for on Apply readBufferSize to the copy loop and split it per direction #36, covering all four construction-frozen proxy fields, with a route-only control and mutation-checked.
  • A README correction. The text added in a07f42d says a recreate drops established connections. It doesn't — stop() ends the accept loops and sleeps 100 ms but never aborts connection tasks, and the runtime lives in the napi wrap until GC. Sessions keep running on the old buffer sizes until they close, which matters more than the original wording implied: lower readBufferSize on a listener full of long-lived MQTT subscribers and the change reaches almost nothing until those clients reconnect. Now pinned by a test.

The allocator diagnostics used above (malloc_stats, malloc_trim_now, escalated_buffers) are deliberately not in this PR — they live on a local scratch branch, and I'd only land them if we decide the out-of-process measurement is worth building properly.

Claude (Opus 5), with Kris

@kriszyp

kriszyp commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Ran the out-of-process measurement I flagged as the deciding experiment. It decides it, and not in this PR's favour — the premise behind #37 doesn't hold.

The measurement

symphony-server in its own process, driven by a client and echo upstream in a separate one, sampling only the server's VmRSS from /proc. 8000 connections, readBufferSize 64 KiB, one burst each then parked — same shape as bench-copy-memory, with the client's own TLS state (which dominated every earlier number) excluded.

pair escalating static (pre-#41)
1 42,409 B/conn 44,440
2 44,495 44,857
3 44,464 44,073
mean 43,789 44,457

668 B/conn apart, against a model predicting ~128 KB/conn. But the number that actually explains everything is the absolute one: symphony costs ~44 KB per connection in total. If the static arm held 2 × 64 KiB resident per connection it could not possibly be under 131 KB.

Root cause: the buffers were never resident

Sweeping readBufferSize on the static arm, where the whole 128 KB/conn is supposedly held for the connection's life:

static arm RSS/conn
readBufferSize: 65536 44,143 B
readBufferSize: 4096 38,703 B
difference 5,440 B — predicted, if resident: 122,880 B

About 4% of the buffer is ever resident. vec![0u8; n] goes through alloc_zeroed, which for a buffer this size is served by fresh untouched zero pages — no memset, nothing faulted in. And because the copy loop drains to pos == cap == 0 after every write cycle, every subsequent read refills from offset 0. The tail of a 64 KiB buffer is never written, so it never becomes resident.

So tokio::io::CopyBuffer does hold readBufferSize × 2 per connection for the session's life — virtually. The dead weight this PR set out to reclaim was already not costing resident memory. That is why no instrument could find the saving: there was none to find.

It also reconciles the confusing middle of this investigation. mallinfo2's uordblks showed the full ~557 MiB difference between a 4 KiB and 64 KiB static configuration, while RSS showed nothing — exactly right, because uordblks counts allocated bytes and RSS counts touched ones. I was reading a virtual-memory number as if it were a physical one.

Corrections to what I posted earlier today

  • The MALLOC_ARENA_MAX=2 recommendation was wrong. I measured ~10–13 KB/conn (4%) with the in-process benchmark and suggested it as a free win. Out-of-process it is 43,745 → 43,307 B/conn — 438 B/conn, ~1%. The saving I saw was mostly the benchmark client's memory, not symphony's. Not worth deploying for.
  • Earlier I said the escalating path wasn't releasing its buffers. It is — the gauge reads 0 of 16000 escalated directions at steady state. Shrink-on-park works correctly. It just has nothing meaningful to release.
  • And the 95,027 B/conn saving in my first comment remains withdrawn; nothing here recovers it.

Three revisions in a day, all from the same mistake in different clothes: measuring a process that contained the load generator, on an effect smaller than that process's own variance.

Recommendation

Close this, and #37 with it. The mechanism is correct and well-tested, but it optimises memory that was never resident, and it isn't free — it adds two allocations plus a zeroing per burst on the hot path that carries every proxied byte. The gating in f1a0d16 makes it safe rather than valuable.

What actually moves symphony's per-connection memory, measured properly:

lever saving per connection
readBufferSize 64 KiB → 4 KiB (#36, already on main) ~5,440 B
MALLOC_ARENA_MAX=2 ~438 B
escalating buffers (this PR) ~0

#36 is the real lever and it already shipped in 1.2.0. For an MQTT fan-out port-set, a small per-direction readBufferSize is the whole win available here — and since it's per proxy, a replication port-set keeps its large buffers untouched.

Worth salvaging from this branch regardless of the outcome, as they stand on their own:

  • the reload regression test for the four construction-frozen proxy fields (asked for on Apply readBufferSize to the copy loop and split it per direction #36, never landed, mutation-checked, with a route-only control);
  • the README correction — a recreate does not drop established connections, so a readBufferSize edit reaches only new connections, which on a listener full of long-lived MQTT subscribers can mean almost none of them;
  • bench-copy-memory-oop.ts, the out-of-process harness. Any future per-connection memory claim about symphony should be measured with this rather than the in-process benchmarks, which cannot resolve anything smaller than their own client-side noise. That's the durable lesson here.

Happy to open a small PR with just those three if that's the preferred shape.

Claude (Opus 5), with Kris

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copy-buffer memory scales with connection count instead of concurrent transfers

2 participants