perf(moq-pub-mmtp): parallelize publisher pipeline + gated pprof profiling endpoint#45
Merged
Merged
Conversation
…endpoint The publisher ran its three long-lived halves -- session.run(), publish_namespace() (egress/encode), and run_publisher() (multicast ingest) -- as three branches of a single tokio::select!. That is one future = one task, and tokio never parallelizes a single task across workers, so recv -> route -> per-shred open_uni -> encode -> quinn-write serialize on ONE core (~0.97 core, ~90% userspace) regardless of the runtime's worker count. Raising the pod CPU limit only removed CFS throttling; it did not lift this single-task ceiling. Split the three halves onto separate tokio::spawn tasks and race their JoinHandles, so the multi-thread runtime can place ingest and egress on different workers. Producer (SubgroupsWriter) and consumer (SubgroupsReader) already communicate through an Arc-backed watch::State, so this is a scheduling change only: the wire output (objects, groups, subgroups, framing, ordering) is byte-identical and the relay is unaffected. run_publisher stays a single task, so the monotonic group_id assignment (datagram.rs) remains a single ordered point. Teardown is preserved -- the first task to finish (Ok or Err) is propagated exactly as the old select! did, so the external watchdog still respawns on error. Also add an optional on-demand CPU profiler behind the `profiling` cargo feature (off by default) AND the MOQ_PUB_PROFILE_ADDR env var (unset by default): a pprof-rs flamegraph/protobuf endpoint served by tiny_http on its own std thread. With the feature off, the default release build never compiles pprof/tiny_http, so the shipped binary is byte-for-byte unchanged. pprof-rs samples via SIGPROF/ITIMER_PROF (userspace, no CAP_SYS_ADMIN), so it runs under baseline PodSecurity. `--build-arg PROFILING=1` produces a profiling-enabled moq-pub-mmtp image, used to confirm which userspace frames dominate the pub's single-thread core. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
allyblockcast
approved these changes
Jul 19, 2026
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
reviewed head: 9900c59
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (3)
- [gstack/review]
moq-pub-mmtp/src/profiling.rs:190-208—spawn_if_enabled()/serve()bind whatever addressMOQ_PUB_PROFILE_ADDRnames with no code-level check that it's loopback, and the endpoint is unauthenticated (each request can hold the thread for up to 120s of sampling). The double gate (feature off by default, env var unset by default) makes this low-risk today, but nothing stops a future operator from settingMOQ_PUB_PROFILE_ADDR=0.0.0.0:6060on a profiling-enabled image and exposing an unauthenticated CPU-sampling DoS surface pod-wide. Consider a defensive check that refuses/warns loudly on a non-loopback bind address. - [native-codex]
moq-pub-mmtp/src/main.rs:142-150—.abort()on the two losingJoinHandles is best-effort: since the three halves now run on genuinely separate OS threads (not cooperative branches of oneselect!), a losing task can still be mid-execution of synchronous, side-effecting code (e.g. aquinnwrite) at the momentabort()is called, and only stops at its next.awaitpoint. The old single-taskselect!guaranteed losing branches were always suspended at a poll boundary. In practice this window is harmless here becausemainreturns and the whole process/runtime exits immediately after, but it's a real semantic shift worth a one-line comment if this pattern gets reused somewhere teardown timing matters more. - [pr-review-toolkit:tests] No test exercises the new spawn/select/abort teardown path itself (e.g. that a
JoinErrorfrom a panicking task correctlybail!s, or that the first task to error wins the race). The existing 64 tests are unchanged coverage overmain.rs's previous logic. Given the PR already gates the actual production rollout on a manual soak + watchdog-respawn validation, this is acceptable to defer, but worth a lightweight unit test around theOk(inner) => inner?, Err(join_err) => bail!(...)match arm if convenient.
Strengths
- Clear, honest PR description: explains the actual mechanism (one task ≠ parallel across workers), backs the claim with the CFS-throttling observation, and is explicit about what's still gated behind manual validation before prod rollout.
- Wire-safety argument is sound —
run_publisherstays a single task, so the monotonicgroup_idordering point is preserved; the split is genuinely scheduling-only. - Profiling feature is well isolated: default
cargo build --releasenever touchespprof/tiny_http, verified via feature-gatedmod profiling,cfgon the spawn call, and a separate opt-in Dockerfile stage that only runsif [ -n "$PROFILING" ]. Cargo.lockadditions are exactly pprof/tiny_http's expected transitive deps (inferno, symbolic-demangle, chunked_transfer, etc.) — no unexpected or suspicious packages.- Docker layering keeps the profiling rebuild after the default build/copy and before the final
COPY --from=builder, so the opt-in binary swap can't accidentally leak into the default image or vice versa.
Recommended Action
- No blocking issues — safe to merge on review grounds.
- Suggestions are optional hardening; address opportunistically.
- Remember the PR's own stated gate: don't auto-roll the parallelized pub to prod off this merge without the flamegraph + soak validation it describes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two changes to
moq-pub-mmtp, one combined PR:1. Parallelize the publisher pipeline (the headline). The publisher ran its three long-lived halves in a single
tokio::select!:That is one future = one task, and tokio never parallelizes a single task across workers — so
recv_from -> route -> per-shred open_uni -> encode -> quinn-writeall serialize on one core (~0.97 core, ~90% userspace) no matter how many workers the runtime has. (This is why bumping the pod CPU limit 1→2 core only removed CFS throttling; it did not lift the ceiling.)This PR spawns the three halves as separate tasks and races their
JoinHandles, letting the multi-thread runtime place ingest and egress on different workers.2. Optional on-demand CPU profiler behind the
profilingcargo feature (off by default) and theMOQ_PUB_PROFILE_ADDRenv var (unset by default) — the instrument used to confirm which userspace frames dominate the pub's single-thread core.Wire-safety (relay unaffected)
The producer (
SubgroupsWriter) and consumer (SubgroupsReader) already communicate through an Arc-backedwatch::State, so the split is a scheduling change only — the wire output (objects, groups, subgroups, framing, ordering) is byte-identical.run_publisherstays a single task, so the monotonicgroup_idassignment (datagram.rs) remains a single ordered point. Teardown is preserved: the first task to finish (Ok or Err) is propagated exactly as the oldselect!did, so the external watchdog still respawns on error.Profiling: default build is unchanged
With the feature off, the default
cargo build --releasenever compilespprof/tiny_http, so the shipped binary is byte-for-byte unchanged. pprof-rs samples viaSIGPROF/ITIMER_PROF(userspace, noCAP_SYS_ADMIN), so it runs under baseline PodSecurity.tiny_httpserves on its own std thread (cannot perturb the tokio hot path).Deploy note (do NOT auto-roll from this merge)
This PR lands the code only. The prod shred pub is digest-pinned/operator-managed (not Argo-auto-rolled), so merging here does not deploy. The parallelization roll is intentionally gated on: (a) a flamegraph confirming the userspace core is the produce/egress split rather than per-shred
open_uni(which is wire-locked and out of scope), and (b) a >1h no-crash soak + disconnect→watchdog-respawn validation, off a digest-pinned image with clean-slate recovery staged.Testing (local, on this branch)
cargo check -p moq-pub-mmtp(default) ✅ — proves the three futures areSend + 'staticon moq-transport 0.15cargo check -p moq-pub-mmtp --features profiling✅cargo clippy -p moq-pub-mmtp --no-deps(both feature states) ✅ no warningscargo test -p moq-pub-mmtp✅ 64 tests passcargo fmt --check✅ ; REUSE covered by themoq-pub-mmtp/**glob🤖 Generated with Claude Code