Repository navigation
perf(moq-net): readers of a streaming frame share its byte owners - #7509
Conversation
Quest-Claim: 819e5b01da838433
Readers slicing a frame still arriving shared nothing: every read boxed a fresh owner over the segment. The first read past each publication now builds one owner over every published byte, cached beside the segment, and later readers slice it. Growth drops the cached owner with the old segment, so it never outlives the slices cut from it. Segments are left uninitialized, since only their published prefix is read and every byte of it was written first. A frame chain is boxed so the cached owner adds 8 bytes to each frame's content, not 32. The frame bench sweeps publishers, readers, and arrival chunks, with FRAME_ALLOCS=1 printing allocations per read; the group bench reads a streaming frame from readers across threads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: cb9a93a
No actionable bugs found in the changed areas. The direction is sound: caching the published-prefix owner addresses allocation fanout at its source, and dropping that cache on growth preserves old-slice ownership without retaining superseded segments. The small lock-based cache is preferable here to adding lock-free reclamation machinery; the fixed-reader locking and boxed-chain tradeoffs are explicit. No production public API or wire change found.
Checked publication ordering, initialized-prefix bounds, overlapping deliveries, growth/sealing/freezing, the borrowed consumer path, and benchmark scaffolding. Static GitHub review only; I did not run tests, Miri, or benchmarks or independently reproduce the reported performance measurements.
(Written by OpenAI)
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 72.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the main change: readers of a streaming frame share one cached byte owner, which is the core of the diff in fuzz.rs, model/frame.rs, and the benches. It uses the conventional perf(moq-… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description directly explains the shared byte-owner implementation, allocation and instruction improvements, benchmark changes, validation, and scope. These topics match the reported … Full details: Title check
-
Explanation
The title names the main change: readers of a streaming frame share one cached byte owner, which is the core of the diff in fuzz.rs, model/frame.rs, and the benches. It uses the conventional perf(moq-net) prefix and is a single concise sentence. Nothing in the title is off-topic.
Full details: Description check
-
Explanation
The pull request description directly explains the shared byte-owner implementation, allocation and instruction improvements, benchmark changes, validation, and scope. These topics match the reported changeset.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 72.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 18 functions across 4 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![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 clarifying note on a safety comment.
Reviewed changes
- Shared byte owner.
FrameContent::slicecaches oneBytesowner per publication beside the storage (FrameStorage::Fixed { segment, shared }andOpen { segment, shared }), rebuilt whenwrittenadvances and dropped whenOpen::reservegrows the segment. Sealed chain segments keep one owner each andfreezereuses the cached owner. - Uninitialized segments.
Segment::newnow allocates withBox::<[u8]>::new_uninit_slice; only the published prefix is read. Verified there is no read path that observes a byte before it is written: everyFilled::lenis bounded by thecontent.writtenwatermark, storedReleaseafter the bytes and loadedAcquireby readers. TheFixedarm'ssharedmutex does not synchronize with writers, but the watermark pairing does and the concurrent write region is disjoint from[0, len). - Boxed chain and read-path borrow.
FrameStorage::Chainis boxed andpoll_read_chunk_onceborrows&self.sourceinstead of cloning itsFrameBuf. - Benches and helper. New
frame_read(with a counting-allocatorFRAME_ALLOCSmode) and threadedgroup_stream_readbenches;FrameRecv::with_readersadded andrunnow returns the chunk count. Both benches are reached by nightly'scriterion net ... --bench '*'. - Docs. Deletes
quest/m2/perf/frame-read-owners.mdand its index bullet, and updates the zero-fill sentence inquest/m2/quic-io-boundary.md.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
codex / Codex land fixer note:
(written by codex / Codex) |
|
codex / Codex land fixer: const published = new Map<string, string>();
for (const dir of manifest(root).workspaces ?? []) {
const pkg = manifest(join(root, dir));
if (pkg.name.startsWith("@moq/") && pkg.scripts?.release) published.set(pkg.name, join(root, dir));
}
const declared = Object.keys(manifest(here).devDependencies ?? {}).filter((name) => name.startsWith("@moq/"));
const missing = [...published.keys()].filter((name) => !declared.includes(name));
const extra = declared.filter((name) => !published.has(name));
if (missing.length || extra.length) {
throw new Error(
`${here}/package.json must declare exactly the published packages: missing [${missing}], extra [${extra}]`,
);
}
const out = join(here, "../../lib/js/api");
// `--cached` reuses the last reference while no input has changed since, unless
// MOQ_API_REFERENCE=regenerate; the dev server passes it, a build never does.
const stamp = join(out, ".inputs");
const inputs = newest([...published.values(), here]);
const cached = process.argv.includes("--cached") && process.env.MOQ_API_REFERENCE !== "regenerate";
if (cached && existsSync(stamp) && Number(readFileSync(stamp, "utf8")) >= inputs) {
console.log("API reference: unchanged, reusing it; MOQ_API_REFERENCE=regenerate forces it");
} else {
const started = performance.now();
await generate({ root, packages: [...published.values()], out, docs: join(here, "../..") });
writeFileSync(stamp, String(inputs));
console.log(
`API reference: ${published.size} packages in ${((performance.now() - started) / 1000).toFixed(1)}s`,
);
}
}
// The newest modification time of any file or directory under `dirs`, which a deleted file
// changes too, skipping installs and build output.
function newest(dirs: string[]): number {
let found = 0;
const walk = (path: string) => {
const stat = statSync(path);
found = Math.max(found, stat.mtimeMs);
if (!stat.isDirectory()) return;
for (const entry of readdirSync(path)) {
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.
llmstxt » vitepress-plugin-llms initialized (SSR build) with workDir: /tmp/quest-land-7530/doc
llmstxt » Build started, file collection cleared
llmstxt » Skipping LLMs docs generation in SSR build
�[32m✓�[0m building client + server bundles...
- rendering pages...
�[32m✓�[0m rendering pages...
build complete in 184.13s.
ELAPSED %CPU RSS COMMAND
codex
All checks passed, including #7530’s documentation build and 438 relay tests with io_uring enabled. I’ll requeue all three PRs unchanged, name the intermittent peer-close tests in the result, and leave blame unset because the evidence does not identify a responsible PR.
exec
/bin/bash -lc "python3 - <<'PY'
import json
from pathlib import Path
flake = 'intermittent landing tests: a peer close drains the overtaken group over moq-lite-02, moq-lite-03, and moq-lite-04'
results = [
{
'number': 7553,
'action': 'requeue',
'reason': 'Healthy with base; passed bun run --cwd js/net build, bun run --cwd js/net check, and bun run --cwd js/net test (2694 passed); ' + flake,
'comment': 'The landing log reports peer-close drain failures in the existing MoQ Lite tests. This PR changes only IETF join tests and quest documents. The failing tests passed on base, the full base net suite passed (2689 tests), and the full net suite with this PR and base passed (2694 tests). Requeueing unchanged; please follow up on the intermittent peer-close tests for moq-lite-02, moq-lite-03, and moq-lite-04.'
},
{
'number': 7509,
'action': 'requeue',
'reason': 'Healthy with base; passed cargo build --locked -p moq-net --features fuzz --bench frame --bench group and cargo nextest run --locked -p moq-net --tests --test-threads 4 (3232 passed); unchanged JS passed bun test --cwd js/net --only-failures on base; ' + flake,
'comment': 'The Rust changes and both modified benchmarks build successfully. All 3232 moq-net tests passed with Nextest. An initial cargo test --locked -p moq-net --all-targets run encountered the existing setup::tests::credentials_stay_out_of_traces global tracing-subscriber conflict; the repository uses Nextest to isolate tests in separate processes. No source changes were needed. The original landing failure was in unchanged JavaScript peer-close tests, which passed in the full base net suite.'
},
{
'number': 7530,
'action': 'requeue',
'reason': 'Healthy with base; passed cargo build --locked -p kio -p moq-uring -p moq-relay, cargo test --locked -p kio -p moq-uring -p moq-relay, cargo nextest run --locked -p moq-relay --features io-uring --tests --test-threads 4 (438 passed), and bun run --cwd doc check; unchanged JS passed bun test --cwd js/net --only-failures on base; ' + flake
}
]
Path('quest-land-result.json').write_text(json.dumps(results, indent=2) + '\\n')
assert len(results) == 3 and {r['number'] for r in results} == {7553, 7509, 7530}
pinned = {7553: '378a07b7d0c390b6044c0802e57e1c80e7f56e56', 7509: '94a1d2be089a2738e8f7c412921d1833a87cf334', 7530: 'cbe4a2bc680f0dc568ebdc6e015cd7828d1e2c1b'}
import subprocess
for number, head in pinned.items():
assert subprocess.check_output(['git', 'rev-parse', f'pr-{number}'], text=True).strip() == head
print('Result validated; all PR branch tips unchanged.')
PY
git status --short && git -C /tmp/quest-land-7509 status --short --untracked-files=no && git -C /tmp/quest-land-7530 status --short --untracked-files=no && git -C /tmp/quest-land-7553 status --short --untracked-files=no" in /work
succeeded in 9759ms:
Result validated; all PR branch tips unchanged.
?? quest-land-result.json
codex
Wrote [quest-land-result.json](/work/quest-land-result.json), requeueing all three PRs unchanged.
Builds and tests passed. The result names the intermittent MoQ Lite peer-close tests. No PR was blamed; all branch tips remain unchanged.
tokens used
85,072
Wrote [quest-land-result.json](/work/quest-land-result.json), requeueing all three PRs unchanged.
Builds and tests passed. The result names the intermittent MoQ Lite peer-close tests. No PR was blamed; all branch tips remain unchanged.
fixer result written
(written by codex / Codex) |
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Finishes quest/m2/perf/frame-read-owners.md.
Problem
Reading a streaming frame boxes a fresh
Bytesowner per reader per read. A relay pays those allocations for every subscriber after every publication of new bytes.Approach
Impact
FrameRecv::with_readersadds readers;runreturns the number of chunks read.Alternatives
The old per-reader owner keeps Fixed reads free of an owner-cache lock but allocates per reader, defeating the quest's goal. A lock-free cache would require safe reclamation machinery or retaining superseded owners. The selected design shares one owner per publication while preserving reclamation. Chain boxing limits the per-frame metadata cost; no public API or backpressure redesign is needed.
Measurements
Remeasured on 2026-11-02 against main
693a0ec229, with identical benchmark fixtures added to the base. Base and PR use the same compiler, optimized profile and controls.FRAME_ALLOCS=1passed all 48 combinations: upfront/grow, 16 KiB/2 MiB, 1/4 publishers, 1/16/128 readers, and 4/32 arrivals. Readers retain all old chunks across growth. Counts subtract identical no-reader ingest. At 128 readers and 32 arrivals, for both publisher counts:That is about one owner allocation per publication, rather than per reader. Allocated owner bytes per read drop from 24.0 B (16 KiB) or 22.5 B (2 MiB) to 0.2 B.
Marginal user instructions/read with 128 readers, measured as
(instructions at 32 arrivals - instructions at 4 arrivals) / (28 * publishers * readers). Three alternating samples per shape, CPU 0 pinned, existing Criterion--testcontrols; setup and process overhead are subtracted by the arrival difference:The 128-reader cases use 44.9-48.6% fewer marginal instructions/read. The single-reader control costs 0.6-18.4% more, depending on shape. For example, 16 KiB upfront with 1 publisher is 1,406 to 1,664 instructions/read; with 4 publishers it is 1,567 to 1,638. This replaces the older, smaller single-reader estimate rather than claiming every workload improves.
group_stream_readpassed all 12 smoke shapes, then measured three runs per side with--bench --quick: arrivals {4,16}, threads {1,4,16}, readers {16,256}. Two pairs alternated base/current then current/base. Point estimates in microseconds per round:This shared-host wall clock is mixed and noisy; no general wall-clock speedup is claimed. The 4-thread/16-reader/16-arrival case has a repeatable cost (3.10-3.35 us base, 4.23-4.35 us PR). The 1-thread/256-reader and 4-thread/256-reader cases improve. The 16-thread results vary substantially, including large base A/A spread. The intended allocation and high-fanout instruction improvements hold, with these measured contention tradeoffs.
Original implementation profile, retained as the zero-fill evidence:
perf recordonframe_recv/upfront/size=16384/streams=64showed__memset_avx2_unaligned_ermsat 3.0% of cycles, versus 5.8% for the payload copy. Sincerep stosbcounts as one instruction, the author also isolated the fill usingGLIBC_TUNABLES=glibc.cpu.x86_rep_stosb_threshold=0x7fffffff:glibc.cpu.x86_rep_movsb_threshold=0x7fffffff:The recorded profile meets the quest's condition to keep the zero-fill optimization only when the profile shows its cost. Both benchmark targets run nightly through moq-net's
--bench '*'leg.Validation
quest gate just ci origin/main: passed, 9,476 Rust tests passed, 109 skipped, plus selected formatting, documentation, quest and tooling checks. The initial inherited environment failed Cargo launch with E2BIG; a freshnix develop --ignore-environment --keep HOME --keep PATHfixed the environment, without changing source or test settings.backend::quic_client_close_drains_finished_trackpassed in the full gate. The shared failure addressed by moq-dev/moq#7634 did not reproduce. This PR neither incorporates nor duplicates that repair.quest check --base origin/main --branch quest/m2/perf/frame-read-owners: passed.Review
Claude Code, Claude Opus 5.5, medium effort,
quest review --harness claude --base origin/main, session677add45-64f9-4609-9d6a-fc30a34c4de6, round 2, head57285e448aagainst62ccb69bac: no findings.Round 1 dispositions, accepted in round 2:
18588e5dd3: Fixed bytes rely on the Release/Acquire publication boundary, rather than its reader-only owner lock.Follow-ups
None required. No new API or scope choices were made in this iteration.
(Written by GPT-6.1 Sol)