perf(moq-pub-mmtp): parallelize publisher pipeline + add gated pprof …#46
Conversation
…endpoint (#45) 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
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.
Looks good
Reviewed the task-spawn refactor in main.rs and the new gated pprof endpoint in profiling.rs. No Critical or Important issues found.
main.rs: the switch from a singleselect!over three inline futures to threetokio::spawn'd tasks raced via&mut JoinHandleis correct —#[tokio::main]here uses the default multi-thread flavor (tokio = { features = ["full"] }, noflavor/worker_threadsoverride), so this actually gets the parallel scheduling the PR claims. Teardown semantics are preserved:.abort()on all three handles after the race (including the winner, a harmless no-op) mirrors the oldselect!'s implicit drop of the losing branches, and aJoinErroron the winning task is converted to abail!rather than silently swallowed. CI's release build (buildcheck) confirms theSend + 'staticbounds hold for the movedsession/publisher/tracks_reader/router/tracks_writer.profiling.rs/Cargo.toml/Dockerfile: the feature-gate (profiling, off by default) plus runtime env-var gate (MOQ_PUB_PROFILE_ADDR) is layered correctly —#[cfg(feature = "profiling")]on themod profiling;declaration means the default release build never even parses this file, matching the "byte-for-byte unchanged" claim. The endpoint runs on its ownstd::thread, so a 30–120s blocking sample can't stall the tokio hot path.
Suggestions (1)
- [gstack/review]
moq-pub-mmtp/src/profiling.rs:38—spawn_if_enabled()binds to whateverMOQ_PUB_PROFILE_ADDRis set to with no validation that it's loopback. The surrounding comments (both here and inCargo.toml) describe the endpoint as "bind loopback only," but nothing in code enforces that — an operator setting e.g.0.0.0.0:6060on a profiling-enabled image would expose unauthenticated CPU-profile capture (and up to 120s-blocking DoS via repeated requests) on a routable interface. Consider a startup check that warns loudly (or refuses to bind) when the parsed host isn't127.0.0.1/::1/localhost, so the documented intent is actually guaranteed rather than operator-trusted.
Strengths
- Comments throughout explain non-obvious why (single-task-can't-parallelize-across-workers, why
abort()after already racing is safe, why the profiler thread can't perturb the hot path) rather than restating code. - The Dockerfile's optional second build stage correctly overwrites
/usr/local/cargo/bin/moq-pub-mmtpbefore the finalCOPY --from=builder, so--build-arg PROFILING=1cleanly swaps the binary without touching the default build path.
Recommended Action
- Consider the loopback-binding suggestion above before shipping
PROFILING=1images to any environment where the debug port could be reachable off-host.
Reviewed head: d2eb1eb
|
Superseded for the CPU lever by #47. #47 fixes the root cause (unbounded waker-Vec growth in If the gated pprof endpoint in this PR is still wanted for profiling cloudflare#141, suggest splitting it out to a pprof-only PR and closing the parallelization half. Either way, don't roll this image for CPU — see the relay req-id drift deploy note I left on #47 (applies to any pub image roll). |
…endpoint (#45)
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
profilingcargo 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=1produces a profiling-enabled moq-pub-mmtp image, used to confirm which userspace frames dominate the pub's single-thread core.