[kixelated]

Finishes quest/m2/perf/lite-request-allocs.md, the two follow-ups from moq-dev/moq#6952.

Problem

• Path::concat (so Path::join for any result over 22 bytes) built a String, then copied it into the path's Arc<str>: two allocations where one is needed.
• The lite request serve (TRACK, SUBSCRIBE, FETCH) copied the track name twice: once into the owned message, and again into RequestServe::track for its logs.

Approach

• concat joins a long path in an arrayvec::ArrayString on the stack (uninitialized, like ietf::namespace::from_tuple), so the shared Arc<str> is the only allocation. The inline branch (22 bytes or less) is unchanged.
• RequestServe no longer copies the name. While the request resolves, its logs read the name from the message (RequestServe::track()). As the request starts, Request::take_track moves the message's Cow into the log context, and Request::start takes the name as &str for the track lookup. Nothing reads the message's track after start. A graceful request (SUBSCRIBE) whose requester FINs before it starts moves the name into the log context the same way, so its complete log still names the track.
• moq-net-tests/tests/announce_allocs.rs gains two tests:
  • a_joined_path_allocates_only_its_buffer: 0 allocations joining a short path, 1 for a long one.
  • a_lite_request_keeps_one_copy_of_its_track_name: a full mock session subscribes on moq-lite 03 and 07. It counts the allocations sized exactly like an odd-length track name, only while the publisher's driver polls (a Watched wrapper), so the subscriber's own copies don't count.

Measurements

All deterministic: allocation counts, and user-space instruction counts pinned to one E-core. Before is main at 2d13685.

announce_allocs (the new tests, run against both):

	main	this PR
Path::join, 10 / 47 byte result	0 / 2	0 / 1
publisher's track-name copies, moq-lite 03 (SUBSCRIBE)	2	1
publisher's track-name copies, moq-lite 07 (TRACK + SUBSCRIBE)	4	2

cargo bench -p moq-net --features fuzz --bench path (allocations per op; rows that changed):

	main	this PR
path/join/medium (26 bytes)	2.00	1.00
path/join/long	2.00	1.00
decode/announce_lite07/medium	4.19	3.60
decode/announce_lite07/long	6.00	5.01
origin/request_rooted_str/long	348	340

The other 28 rows are unchanged.

Instructions per join (perf stat -e cpu_atom/instructions/u, 10M joins, net of an empty loop, using a standalone copy of Repr and both concats):

	main	this PR
short (10 bytes)	80	82
medium (22 bytes)	78	81
long (47 bytes)	352	233

Choices

• The inline branch stays hand-written. Routing every join through the stack buffer and Repr::copy_bounded was smaller code, but cost about 42 more instructions per inline join (122 vs 80). Only the shared branch uses the ArrayString.
• The name moves at start, not at decode. Moving it out at decode would leave the message's track empty while RequestState::Resolve holds it, so the started log's request = ?msg would print track: "". Until start, the message keeps the one copy and RequestServe::track() reads it there.
• Request::start takes a fourth argument (track: &str). The trait is private, and a struct for one call site would add more than it saves.
• The session test does its own handshake instead of using support::harness::connect_mock, so it can wrap the server's driver in Watched. The harness has no hook for that, and adding one for a single test seemed worse.
• No just bench BASE run. The host is shared and loaded, so wall-clock relay benches would be noise. The perf epic's before/after is the deterministic allocation and instruction counts above.

Impact

• Public API: none. Path::concat is pub(crate), and Request is private to lite::publisher.
• Wire: none.

Checks

• quest gate nix develop --command just rs check-test -p moq-net -p moq-net-tests (nextest with clippy as the wrapper, then fmt, doc, shear, sort): 3248 passed, on 5844c41 after merging main at 70821da.
• quest check --base origin/main: ok.
• I didn't run the dependents of moq-net locally. Both changes are crate-private and behavior-preserving.

Merge from main

• No textual conflicts. One semantic conflict: moq-dev/moq#7027 moved moq-clock into kio::clock, so the new session test now uses #[kio::test] instead of #[moq_clock::test].

Review

• Reviewer: Codex (quest review), gpt-6.1-sol, medium effort.
• Head: 5844c41, round 1 (new session).
• Findings: none.
• Online: Pullfrog's log-only finding (a SUBSCRIBE that FINs mid-resolve logged track="") is fixed in 5844c41, as it suggested. The OpenAI review on a7a6f79 had no findings.

Follow-ups

• The lite subscriber also copies the track name twice per subscription, in SubscriberDriver::poll and TrackServe::prepare_establish. The new test's backtraces showed both. That could be a quest of its own if it matters.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You'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 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7ce6e607-ed03-4cc1-bf69-54d6b7216637

Commits

Reviewing files that changed from the base of the PR and between 70821da and 5844c41.

Files selected for processing: 5

• quest/m2/perf/README.md
• quest/m2/perf/lite-request-allocs.md
• rs/moq-net-tests/tests/announce_allocs.rs
• rs/moq-net/src/lite/publisher.rs
• rs/moq-net/src/path/mod.rs

• 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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: a7a6f79

No actionable bugs found. The stack-buffer join retains the existing byte bound and inline path; moving the request's track name at start preserves lookup inputs without another owned copy. Subsequent SUBSCRIBE/FETCH processing does not depend on the emptied message field.

Direction: these are appropriately scoped ownership optimizations. Reusing ArrayString and keeping the change inside the private request machinery avoids a new public API or wire change; the focused allocation tests are useful regression coverage.

Verification: static inspection of the changes and relevant callers/state transitions only. I did not run tests or benchmarks or independently reproduce the reported allocation/instruction measurements.

(Written by OpenAI)

[pullfrog]

ℹ️ No critical issues — one minor, log-only suggestion below.

Reviewed changes — full diff at a7a6f79, plus the two new allocation tests run locally.

• Path::concat long branch — joins on a stack arrayvec::ArrayString::<Path::MAX_BYTES> instead of a String, so the shared Arc<str> is the only allocation; inline branch untouched. Correct: len <= MAX_BYTES is checked before the pushes, so nothing can overflow the buffer.
• Lite request track name — Request gains take_track and a track: &str arg to start; RequestServe::track and the track() reader move the name from the message into the log context at start instead of copying it at decode. All four Request impls and the one unit-test call site are updated; the message's emptied track is never read after start.
• announce_allocs.rs — a_joined_path_allocates_only_its_buffer and a_lite_request_keeps_one_copy_of_its_track_name (the Watched driver scopes the count to the publisher's poll). Both pass; moq-net lib tests compile.
• Quest bookkeeping — lite-request-allocs.md deleted and its README bullet removed; no dangling references.

ℹ️ RequestServe::track loses the name when a graceful request FINs mid-resolve

track() reads the name from the message only while the state is Resolve, and from self.track (populated by take_track at start) otherwise. But poll_serve can leave Resolve without starting: when the requester FINs while resolving, line 1730 replaces the state with Finish and drops the message, so self.track stays empty. SubscribeServe is the only GRACEFUL impl, so a subscriber that closes before its broadcast resolves now logs track="" in {} complete (and any later line), where before this PR the name was captured at decode. No behavior beyond logging: track() is the sole reader of self.track.

Technical details

# Preserve the track name across the FIN-during-resolve path

## Affected sites
- `rs/moq-net/src/lite/publisher.rs:1667-1671` — `track()` returns `&self.track` for any non-`Resolve` state.
- `rs/moq-net/src/lite/publisher.rs:1730` — `self.state = RequestState::Finish { finished: false };` drops the `Resolve` message without calling `take_track`.

## Required outcome
- After a graceful request FINs before `start`, the request's log context still names the track, matching the pre-PR behavior and the JS publisher (`js/net/src/lite/publisher.ts:1510` keeps logging `msg.track`).

## Suggested approach (optional)
- Before overwriting the state at line 1730, when it is `Resolve { msg, .. }`, set `self.track = R::take_track(msg)` so the one copy moves into the log context exactly as it does at `start`.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Fixed Pullfrog's log-only finding in 5844c41: when a graceful request FINs while still resolving, poll_serve now moves the name out of the message (R::take_track) before dropping it, so the complete log names the track, as start already did.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 5844c41

No new actionable bugs found. Compared the base-relative changes with my review at a7a6f79, separating the main merge from the follow-up fix.

Fixed: Pullfrog's FIN-during-resolve logging finding. In rs/moq-net/src/lite/publisher.rs:1731–1735, the transition to Finish now moves the unresolved message's track name into the log context; an already-serving request retains its existing name. The allocation test also correctly uses the current kio test harness.

Direction: the small ownership transfer addresses the missing transition without adding another allocation or abstraction. The original scoped optimization remains sound in the changed areas.

Verification: static inspection of the old/new base-relative diffs, request state transitions, and test harness only. I did not run tests or benchmarks; the FIN logging fix has not been runtime-verified by me.

(Written by OpenAI)

[pullfrog]

✅ No new issues found.

Reviewed changes — the commits since the prior pullfrog review at a7a6f79.

• Closed the FIN-during-resolve log gap — poll_serve now moves an unresolved Resolve message's track name into self.track before switching to Finish, so a SubscribeServe that FINs mid-resolve still logs its track name. This is the only Resolve→non-Resolve transition that bypassed take_track; the other error exits already logged through track(), which reads the message while the state is Resolve.
• Renamed the new session test's harness attribute — #[moq_clock::test] → #[kio::test], matching main moving moq-clock into kio::clock.
• Merged main — a perf-quest README bullet swap; the finished quest file stays deleted.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}