Repository navigation
perf(net): copy small whole frames out of shared receive buffers - #7522
Conversation
Quest-Claim: 5714f9ecc7a5db3b
A frame that arrives whole in one transport chunk is cached as that chunk, a slice of the datagram's receive allocation. The new `backing` bench receives mixed audio and video through the session's frame reader with per-chunk, per-datagram and moq-tokio-style 64 KiB receive allocations, evicts video groups while audio stays, and prints the live heap retained against Pool::used(). With 64 KiB allocations it retains 1.9x with nothing evicted and up to 33x once the video is gone. The quest is rewritten to the remaining fix, which waits on choosing the copy rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
Reader::poll_read_frame cached a frame that arrived in one transport chunk as that chunk, a slice of the QUIC receive allocation it shared with other datagrams. A small audio frame kept up to 64 KiB alive while the pool billed only the frame: 1.9x the bill with nothing evicted, 33x once the video groups beside it were evicted. A whole frame of at most COPY_WHOLE (2 KiB) is now copied out, which covers a QUIC datagram's stream payload (about 1.45 KB). The backing bench returns to the control in every case. Larger frames, including the in-memory transport's, still cross as the publisher's allocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: de08552b6c47a39fd42a176971995fa24b34dc67.
No actionable bugs found. The whole-frame copy preserves lease consumption and receive-credit release, leaves fragmented-frame assembly unchanged, and retains sharing above 2 KiB. The threshold-boundary test and shared-backing benchmark target the ownership problem directly.
Overall direction is sound: copying at cache ingress is a small, focused alternative to changing every transport's allocation policy or expanding the lease API. The accepted threshold deliberately trades extra small in-memory copies for avoiding default-MTU receive-buffer retention; jumbo paths remain outside that guarantee. No production public API or wire change found; the new harness is fuzz-feature-only.
Verification limits: static GitHub inspection of the change and relevant reader, frame, lease, caller, and nightly-benchmark paths. I did not run tests or benchmarks or independently reproduce the reported performance numbers.
(Written by OpenAI)
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one note on the nightly guard's coverage.
Reviewed changes
Reader::poll_read_framecopies small whole frames: a frame that arrives whole in one chunk and is at mostCOPY_WHOLE(2 KiB) is copied out of it (Bytes::copy_from_slice+lease.advance); larger frames still cross as the chunk. One deliberate copy, sincewrite_deferredstores a whole-frame chunk directly.- New test
a_small_whole_frame_is_copied_out_of_its_chunkpins 1 B / 2 KiB as copies, 2 KiB + 1 as the chunk, and the rest of the datagram still leased. - New
fuzz::GroupRecvharness feeds a frame's chunks, as given, through the real reader into a group (fuzz-only,#[doc(hidden)]module). - New
backingbench sweeps tracks × video bitrate × backing (exact/datagram/recv64k) × selective expiry, printing live heap againstPool::used(). zero_copydoc notes the 2 KiB threshold; thecache-backingquest is finished and deleted with its README link removed.
Public API: none (fuzz::GroupRecv is fuzz-only). Wire: none.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
The timed path printed memory only for the keep=all cases, so the nightly run never showed the eviction cases where the shared receive buffers amplified most. The first run of each timed case now prints the memory of its keep=all, keep=quarter, and keep=none cases. Running inside the bench closure follows Criterion's own filter, so a filtered or --exact run prints only the selected case's lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 6d50c88d60c89c4bd8478f0efb3431b19a010529, compared with my prior review at de08552b6c47a39fd42a176971995fa24b34dc67, excluding merged base changes.
No new actionable bugs found. The update fixes the selective-expiry logging gap identified in Pullfrog's finding: each selected timed case now measures all three retention policies once, outside iter_batched, and uses Criterion's filtering rather than duplicating its argument parsing. The existing runner captures stderr, so these measurements reach the retained logs.
Direction remains sound: reuse the existing measurement path without adding timing cases or an arbitrary memory threshold. The ratios remain diagnostic logs, not automatically compared Criterion metrics; the copy-boundary unit test remains the regression assertion. Production behavior/API/wire are unchanged by this update.
Static GitHub inspection only; tests and benchmarks were not run, and the reported ratios were not independently reproduced.
(Written by OpenAI)
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Changes since the prior pullfrog review (de08552b6c): 26f0edea7 plus a conflict-free merge of main. Only rs/moq-net/benches/backing.rs changed in the delta, all benchmark-only.
- Extracted case-name formatting into a shared
name()helper, reused bycases()and the timed loop. - A timed
keep=allcase now measures and prints the memory line for all threekeepvalues on its first run, so the nightly timed run (criterion net ... --bench '*', unfiltered) logs the selective-expiry cases' ratios too. This resolves the prior review's coverage note.
No production, public API, or wire change.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Finishes quest/m2/performance-survey/cache-backing.md and deletes it.
Summary
The quest assumed fMP4 import puts slices of a fragment into the cache. It does not: every sample is copied into its own frame, both unwrapped and in passthrough. The amplification turned up somewhere else, in network ingest.
Reader::poll_read_framecached a frame that arrives in one transport chunk as that chunk. The frame's own storage was used only when the frame spans chunks. Under QUIC a chunk is a slice of a datagram.moq-tokio's endpoint driver copies each receive into one reusedBytesMutand splits datagrams off it (endpoint.rs). Once any slice of that buffer is still held,bytesreallocates 64 KiB (the most original capacity it keeps), and later datagrams fill that buffer back to back. Amoq-uringGRO completion is one allocation as well. So every frame that fits in a datagram (audio, small deltas, catalogs) kept its whole receive allocation alive. That included the video bytes the cache had already copied out, and other connections' bytes. The pool billed only the frame.poll_read_framenow copies a frame that arrives whole and is at mostCOPY_WHOLE(2 KiB) out of its chunk. Larger frames are still cached as the chunk, so a large frame on the in-memory transport still crosses as the publisher's allocation. A new unit test,a_small_whole_frame_is_copied_out_of_its_chunk, pins 1 B and 2 KiB frames as copies and a 2 KiB + 1 frame as the chunk, and checks the rest of the datagram stays leased. Thezero_copyintegration test (64 KiB frames) still passes, and its doc now notes the threshold.backingbench (rs/moq-net/benches/backing.rs): receives 10 s of interleaved media through the session's frame reader into a shared pool, withtracksvideo and audio track pairs. Frames are packed back to back into 1150-byte packets. The sweep covers backing (exactcontrol,datagramasmoq-uringwithout GRO,recv64kasmoq-tokio), video bitrate {150, 1000, 4000} kbps against 64 kbps audio, tracks {1, 4}, and selective expiry: keep {all, quarter, none} of the video groups while every audio group stays.BACKING_ALLOCS=1prints the live heap the cache retains (each allocation counted once, after the group log's deferred frees) againstPool::used(), along with how many frames arrived whole and the allocations and bytes per frame. Timing covers thekeep=allcases. The first run of each prints the same line for it and for itskeep=quarterandkeep=nonecases, so the nightlyrust-benchleg's-p moq-net --features fuzz --bench '*'logs every case's memory. The bench reports the ratio and doesn't assert it. The unit test above is the regression guard.fuzz::GroupRecv: a fuzz-feature harness that feeds a frame's chunks, as given, throughcoding::Reader::poll_read_frameinto a group, the way the lite subscriber does.Public API: none (
moq_net::fuzz::GroupRecvis fuzz-feature only). Wire: none.Decisions
Settled by the maintainer on 2026-11-01:
Reader::poll_read_framewhen a whole frame is at most a fixed threshold. (Implemented.)moq-tokioallocate each datagram separately, asmoq-uringdoes without GRO.Pool::used()as the payload billed, not the heap held.max_udp_payload_sizeof 1472). It is not tuned for jumbo paths. In-memory frames above 2 KiB cross without a copy.Measurements
Every number here is deterministic. Two full
BACKING_ALLOCS=1runs printed byte-identical output. Instructions are user-modeperf stat -e instructions:uwith the process pinned bytaskset -c 7. Three runs per binary agreed within 0.003%, which serves as the A/A control. Base isc1a1412820. Machine: 24-core Linux 7.0, load average 10-15, Nix toolchain,--releasebench profile, glibc malloc.With the fix (2 KiB, on
de08552b6c)BACKING_ALLOCS=1 cargo bench -p moq-net --features fuzz --bench backing. Live heap retained, as a ratio toPool::used():All 54 cases match the
exactcontrol within 4.2 KB, about two of the 2,072-byte one-time allocations that also show up after teardown. 2 KiB holds the same result the 16 KiB candidate did, because a frame that arrives whole fits in one 1150-byte packet. For the same reason, the 2 KiB patch copies exactly the frames the 16 KiB one did in this bench, so the instruction cost below carries over unchanged.In-memory transport,
MEMORY_ALLOCS=1 cargo bench -p moq-net --features fuzz --bench memory, mainbf4e726f66→ this PR: 64-byte frames go from 35.0 to 37.0 allocations per frame (4337 to 4425 bytes), and 16 KiB frames are unchanged at 34.3 allocations (4214 bytes), since they are no longer copied. With the 16 KiB candidate, 16 KiB frames had grown to 20,643 bytes per frame.Before the fix: live heap retained (ratio to
Pool::used())Before this PR's fix.
copiedis the first measured candidate, which copied a frame that arrived whole and was at most 16 KiB.The other 24 cases follow the same pattern. The control's ratio above 1.0 is bookkeeping the pool's per-entry estimate does not cover, not payload. With nothing evicted,
recv64kretains about 1.9x at 1-4 Mbps: the cache's copy of the video, plus the receive buffers that the audio keeps alive. Evicting video groups while audio stays drives it to 10-34x. Every case returned to its starting heap after teardown, within 2-4 KB of one-time lazy initialization.The copy's cost
The copy adds 1.1-1.5 allocations and about 100-210 bytes allocated per frame received, averaged over all frames (
BACKING_ALLOCS=1). User instructions for the whole process, median of 3:An empty run (no case matches the filter) costs 2,499,814 to 2,500,043 instructions. Net of that, the copy adds 16% to the 150 kbps ingest and 7% at 4 Mbps, about 0.3 µs per frame copied. The copy allocates the frame, then allocates again the first time the frame is cloned, because a
Bytesbuilt from aVecpromotes to shared storage on its first clone. Delivery latency moves only by that CPU, since the copy happens in the same poll that hands the frame over.The in-memory transport's frames cross as the publisher's allocation today. With a 16 KiB threshold,
MEMORY_ALLOCS=1 cargo bench -p moq-net --features fuzz --bench memorywent from 35.9 to 37.9 allocations per frame (4358 to 4446 bytes) for 64-byte frames. For 16 KiB frames it went from 35.2 to 37.2 allocations (4235 to 20643 bytes), because those frames are now copied.fMP4 import (scratch harness, not landed)
The harness imported 20 s of mixed H.264-shaped video and 160-byte AAC-shaped audio, built from
bbb.mp4'smoov. It swept fragments of 1 s and 4 s (two GOPs per fragment), video at {150, 1000, 4000} kbps, unbounded versus a pool capped at a quarter of the input (pool-driven eviction), and unwrapped versus passthrough. It reported the live heap afterImport::finishagainstPool::used().fragment()encodes the rebuiltmdatwith mp4-atom'sVec<T>encoder one byte at a time. TheVecgrows by doubling, andBytes::from(Vec)keeps that capacity. With 4 s fragments, 2.0 MB happens to fit just under 2 MiB (1.07x).5dbdb5a564: 1.02x. That PR streams the payload instead of encoding themdat, so it also removes the per-byte loop. I leftfragment()alone rather than conflict with it.Choices made without asking
moq-tokio'sreserve/extend_from_slice/split_tosequence andmoq-uring's per-completionBytesMut::fromagainst realbytes, then feeds the slices through the real reader and cache. A loopback QUIC run would batch nondeterministically. The packet model ignores QUIC frame headers and per-packet overhead.Error::Evicted, because the pool's debt evicts every track in proportion to what it wrote and so cannot keep audio while dropping video.keepsets the share of video groups that stay.crossbeam-epochas a dev-dependency. It is already in the lockfile throughkio. The bench flushes the group log's epoch-deferred frees before reading the live heap; without that, frees landed in the next case.Bytes::copy_from_slice, the measured patch, rather than writing the slice into the frame's segment storage, so a lease is still never held acrosswrite_deferred.keep=allis timed (18 cases), to keep the nightlyrust-benchleg short. A timed run still prints every case's memory line, untimed, from inside the selected case's bench closure. That way the lines follow Criterion's filter, includingbench/run.sh's--save-baselineand--exactruns.BACKING_ALLOCS=1prints them without timing.Follow-ups
MEMORY_ALLOCS=1 cargo bench -p moq-net --features fuzz --bench memoryno longer hangs on mainbf4e726f66. It now panics atstreams=16/size=1048576(memory.rs:194,unwrap()onErr(Evicted)), with and without this change, so that bench's allocation mode stays broken paststreams=16/size=16384. Onc1a1412820it hung afterstreams=1/size=16384.Checks
On
de08552b6c, after merging mainbf4e726f66:cargo nextest run --locked -p moq-net --all-targets --all-featurespasses: 3222 tests, 13 skipped.cargo clippy --locked -p moq-net --all-targets --all-features -- -D warningsis clean, andcargo fmt -p moq-net --checkpasses.backingbench runs every case.quest check --base origin/mainpasses.cargo test --all-featuresfailssetup::tests::credentials_stay_out_of_tracesonly because another test in the same process set the global tracing subscriber first. It passes alone and under nextest, which CI uses.Before the fix, on
fb1f340472,quest gate just checkpassed.Review
Codex
gpt-6.1-sol, medium effort (quest review --base origin/main, round 1), reviewed headde08552b6cand found nothing. That change got a local review because it changes ingest behavior.Since then,
26f0edea76changes only thebackingbench, and the rest is a conflict-free merge of main. Under the simple-review rule (benchmarks only), the local review was skipped for the new head6d50c88d60, which relies on the online reviewers. On that head,cargo clippy --locked -p moq-net --benches --features fuzz -- -D warnings,cargo fmt -p moq-net --check, andquest check --base origin/mainpass. A filtered run ofbacking/tracks=4/video=4000k/keep=all/recv64kprinted all three keep lines (1.01x, 1.02x, 1.17x) and timed only that case.Online: Pullfrog's thread said the nightly run never logs the eviction cases. That is fixed in
26f0edea76. Its question on asserting the ratio is answered: the ratio is reported, and the reader's unit test is the guard. Pullfrog's check fails because of a billing outage, not this PR.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code