Repository navigation
perf(uring): remove avoidable copies on egress, streams, and datagram receive - #5843
Conversation
… receive
- Egress: moq-quic encrypts each GSO train straight into the TX pool's
buffer instead of a scratch Vec that was then copied. `udp::TxBuf` now
derefs to the slot's `Vec<u8>` (empty on acquire) and `udp::Transmit`
drops `len`: the buffer's length is what goes out.
- Stream send: `poll_write_buf` hands moq-quic the chunk via `write_chunks`,
so a `Bytes` source is kept by reference. A new
`moq_quic::SendStream::write_limit` sizes the chunk first, since bytes
taken from a `Buf` cannot be put back.
- Stream receive: `poll_read_chunk` returns moq-quic's `Bytes` (or a split of
the read-ahead backlog) instead of the trait default's 8 KiB copy.
- Datagram receive: one `BytesMut` per completion, split per GRO segment,
instead of one allocation per datagram. `udp::Packet::{payload_mut,
segments}` lose their only caller and go.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Quest outcome: all four copies are removed (egress, stream send, stream receive, datagram receive). Left as a draft because stream send needed a small moq-quic API addition ( Checks run: Perf came from a loaded shared host. Allocations per MiB are deterministic (-88% on all-on). CPU-time throughput is +12 to 17% on all-on and within noise for the other ablations. (written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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 37 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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c91ec99a55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `UDP_SEGMENT` is a u16, so an oversized segment would silently | ||
| // truncate into a tiny stride and explode the implied segment count. | ||
| if len == 0 || len > self.cap || segment == 0 || segment > usize::from(u16::MAX) { | ||
| if len == 0 || segment == 0 || segment > usize::from(u16::MAX) { |
There was a problem hiding this comment.
When a raw UDP caller grows the newly exposed Vec beyond Config::tx_buffer_len, this validation accepts it because the previous len > capacity check was removed. The oversized allocation is then retained in the pool after completion, so up to tx_buffers_max buffers can permanently exceed the documented per-buffer memory ceiling, and sends can exceed the configured maximum GSO-train size. Keep the buffer bounded or reject lengths above tx_buffer_len.
AGENTS.md reference: AGENTS.md:L63-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in the latest commit: send refuses a buffer grown past tx_buffer_len with InvalidInput again, and buffer_grown_past_the_pool_is_rejected covers it.
(written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 440072f392
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| impl std::ops::Deref for TxBuf { | ||
| type Target = [u8]; | ||
| type Target = Vec<u8>; |
There was a problem hiding this comment.
When a caller uses a safe capacity-reducing operation such as shrink_to_fit, *tx = Vec::new(), or mem::take(&mut *tx) and drops the checkout, release_tx stores that undersized Vec back in the slot while poll_acquire only clears it. If the socket is then passed to quic::Endpoint, Driver::flush_one treats the recycled slot as unable to hold a 1200-byte segment and fails the connection. Avoid exposing capacity-changing operations or restore tx_buffer_len capacity when recycling or acquiring the slot.
AGENTS.md reference: rs/AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in the latest commit: poll_acquire hands out every checkout at exactly tx_buffer_len capacity, replacing a slot Vec its last holder shrank or regrew. a_resized_checkout_comes_back_pool_sized covers both directions.
(written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e49790ebf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let segment = match payload.len() > stride { | ||
| true => payload.split_to(stride), | ||
| false => std::mem::take(&mut payload), |
There was a problem hiding this comment.
When UDP_GRO metadata reports a zero stride, which on_recv explicitly anticipates in udp.rs, payload.len() > stride remains true while split_to(0) returns an empty segment without consuming payload. The demux task then spins forever on the worker thread, preventing every connection on that worker from making progress. Reject a zero stride while decoding the completion or normalize it before entering this loop.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. This is a real edge: the old chunks_mut(0) would have panicked, and the new loop would spin. Fixed at the source in the latest commit: RecvMeta::parse treats a zero UDP_GRO size as no stride, so a packet falls back to its payload length, and only an empty datagram can carry a zero stride (the demux loop never runs for it). a_zero_gro_size_is_no_stride covers the parse.
(written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Ready for review. Both decisions are settled as recommended and recorded in the description: keep Codex review raised three findings, all fixed with tests:
Codex's latest pass found nothing (👍). CodeRabbit was rate-limited and didn't review. CI: Check, Test, and io_uring pass. TS validation fails with an mp2 audio hole (24 to 96 ms). It isn't a required check, and the same failure shows up on other branches (15 of the last 40 TS runs). This PR touches none of its trigger paths or the tokio pipeline it runs, and a rerun passed once. (written by Claude Opus 5.5) |
|
Ready to merge, but not yet enqueued: a local permission check blocked the agent from enqueueing, so a maintainer needs to run
(written by Claude Opus 5.5) |
Completes
quest/m2/perf/uring-copies.md(deleted here, along with its references).Summary
The four copies the quest listed, all in moq-uring:
scratchVec and its copy are gone. To make that work,udp::TxBufnow owns the slot'sVec<u8>while it is checked out.poll_write_bufhands moq-quic the chunk throughwrite_chunks, so aBytessource is kept by reference instead of copied bywrite(&[u8]).poll_writegoes through the same path.poll_read_chunkreturns moq-quic'sBytes, or a zero-copy split of the read-ahead backlog, instead of the trait default's 8 KiBBytesMutplus a copy.web::RecvStreamforwards it.BytesMutper completion, split per GRO segment, instead of one allocation per datagram. The last (or only) segment is taken whole: splitting a single datagram made the allocation shared and cost ~7% onno-groin an early run.API and wire impact
No wire change.
moq_quic::SendStream::write_limit() -> usize(new): how many bytes a write would accept right now. This is a QUIC API addition the quest assumed wasn't needed. See decision 1.moq_uring::udp::TxBufnow derefs toVec<u8>, empty on acquire withtx_buffer_lencapacity, instead of a full-capacity[u8]. Fill it withextend_from_slice/resize.moq_uring::udp::Transmit::lenis removed: the buffer's length is what gets sent, so the two can't disagree. A buffer grown pasttx_buffer_lenis refused withInvalidInput, so the configured per-buffer ceiling still holds.moq_uring::udp::Packet::{payload_mut, segments}are removed. Their only caller was the old datagram demux.Measurements
All numbers come from a loaded shared host (load average 19 to 30 on 32 cores, many concurrent agents). Base and new runs were interleaved. Each one is
session_lite(a full moq-lite publisher/subscriber pair on one worker, 1 MiB per iteration) built from anorigin/mainexport and from this branch.Heap allocations per MiB delivered (deterministic: counting global allocator, measured section only; scratch patch, not committed):
Throughput per CPU-second of the worker thread (
CLOCK_THREAD_CPUTIME_ID, both peers on that thread):Wall-clock Criterion runs swung 180 to 420 MiB/s for the same binary, so I don't quote them.
echo_quic(tokio peer on another thread) was noisier still. Re-run on a quiet box before reading anything into the small-mode deltas.Tests
credit::chunked_writes_stop_at_the_window(new, moq-uring integration):Byteslarger than the peer's stream window takes exactly the window and leaves the rest in the caller's buffer;main("moq-quic copied the chunk");poll_closedread-ahead backlog.stream_flow_control/conn_flow_controlassertwrite_limitmatches whatwriteaccepts, and is zero once blocked or reset.just checkpasses (4835 workspace tests plus the rest).Decisions
Settled by the author with the recommended options:
moq_quic::SendStream::write_limit()(option a).write_chunksalone can't honor the trait's partial-write contract: once a chunk is taken from a genericBuf, the part moq-quic refuses can't be put back. The alternatives were to keep copying on stream send, or to add aBytes-specific write to moq-net's transport trait. The name is open to the maintainer at review (capacity()andwritable()were the alternatives).udp::Packet::{payload_mut, segments}removed. They lost their only caller, the crate is experimental, and restoring them is trivial if a raw-UDP consumer wants them.Suggested follow-ups
RecvStream's read-ahead backlog still copies (extend_from_slice). AVecDeque<Bytes>would drop that copy too.BytesMutcopy, which isquest/m2/quic-io-boundary.md'sEndpoint::handlecandidate.🤖 Generated with Claude Code
(written by Claude Opus 5.5)