feat(node): on-demand per-session CPU/memory profiling (#244) - #475
Merged
Conversation
Adds profiling triggerable via API and recording CPU/memory per proof- generation phase, as requested — with one deliberate deviation from the issue's literal "export as pprof or flamegraph-compatible format": the actual MPC compute work for a session runs in co-noir child processes (session::run_proof_generation spawns one per phase — merge_shares, witness_generation, proof_generation), not in this node's own call stack. An in-process sampling profiler (the pprof crate) would see almost nothing, since none of the expensive work executes on this process's stack. See services/node/docs/PROFILING.md for the full rationale. Instead, profiling samples each co-noir child process's OS-reported CPU% and memory on a 200ms interval for the duration of its phase, exported as JSON per phase (duration, peak memory, avg/peak CPU%) rather than the pprof wire format, since pprof's call-graph model doesn't apply to an opaque external process this node has no stack visibility into. - src/profiling.rs (new): ProfileRegistry (which sessions have profiling enabled + what's been collected so far, Arc-backed so it's cheap to thread into the background proof-generation task) and sample_process_until_exit (the sampling loop — spawned as its own task per phase so it runs concurrently with, not blocking, awaiting the child process). - session.rs: new run_profiled() helper wraps each co-noir subprocess call. When profiling isn't enabled for the session it's exactly cmd.output().await — zero extra cost; when it is, it spawns the child with piped stdio, starts a sampler task against the child's pid, and awaits both. Replaces .output() at all three phase call sites (merge_shares, witness_generation, proof_generation — the last inside its existing transient-failure retry loop, so a retried attempt appends another "proof_generation" phase entry rather than overwriting the previous attempt's). - api.rs: POST /session/:id/profile enables profiling for a session (idempotent, must be called before /generate to have anything to sample); GET /session/:id/profile returns the SessionProfile collected so far as JSON, 404 if profiling was never enabled for that session_id. post_generate only threads the ProfileRegistry through to run_proof_generation when the session_id was explicitly enabled — profiling is strictly opt-in per session, so a session nobody asked to profile pays no sampling overhead beyond one registry lookup. - main.rs: NodeState gains a `profiling: ProfileRegistry` field and the new route. No new dependencies — built entirely on sysinfo and tokio::process, both already direct dependencies (sysinfo is already used the same way for the node's own memory metric in main.rs's background updater). Tests (src/profiling.rs, tests module): ProfileRegistry enable/ is_enabled/get semantics (per-session isolation, idempotent re-enable that doesn't clear prior phases, 404-equivalent None for a never-enabled session), phases append rather than overwrite across merge_shares/witness_generation/proof_generation, and one #[cfg(unix)]-gated test spawning a real short-lived `sh -c "sleep 0.3"` child to confirm sample_process_until_exit records a phase with a non-zero duration once it exits (unix-only since this service's production dependency, co-noir, and its Docker deployment already target Linux). Rebased onto upstream/main after this branch was created; that pulled in an unrelated per-phase timeout watchdog (PhaseTimeouts, added as a new parameter to run_proof_generation in the same commit range) — merged cleanly alongside the profile parameter, since the two features don't interact. Verification: no local Rust toolchain available in this environment (link.exe fails compiling proc-macro2/quote build scripts — the same limitation hit on other Rust work this session), so `cargo test -p mpc-node` could not be run here. Verified by close manual review: the three run_profiled call sites were checked against session.rs's own diff to confirm they didn't collide with the newly-merged watchdog deadline checks (which sit on separate lines); ProfileRegistry's private record_phase is called directly from its own tests module (a Rust child module can reach a private parent-module item), which compiles under the same visibility rule this file's own tests already rely on implicitly.
|
@Davidemulo Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
Adds profiling triggerable via API, recording CPU/memory per proof-generation phase — with one deliberate deviation from the issue's literal "export as pprof or flamegraph-compatible format": the actual MPC compute work for a session runs in
co-noirchild processes (session::run_proof_generationspawns one per phase —merge_shares,witness_generation,proof_generation), not in this node's own call stack. An in-process sampling profiler (thepprofcrate) would see almost nothing, since none of the expensive work executes on this process's stack. Seeservices/node/docs/PROFILING.mdfor the full rationale.Instead, profiling samples each
co-noirchild process's OS-reported CPU% and memory on a 200ms interval for the duration of its phase, exported as JSON per phase (duration, peak memory, avg/peak CPU%) rather than the pprof wire format, since pprof's call-graph model doesn't apply to an opaque external process this node has no stack visibility into.Changes
src/profiling.rs(new):ProfileRegistry(which sessions have profiling enabled + what's been collected so far) andsample_process_until_exit(the sampling loop, spawned as its own task per phase).session.rs: newrun_profiled()helper wraps eachco-noirsubprocess call. When profiling isn't enabled it's exactlycmd.output().await— zero extra cost; when it is, it spawns the child with piped stdio and starts a sampler task against the child's pid. A retriedproof_generationattempt (existing transient-failure retry loop) appends another phase entry rather than overwriting.api.rs:POST /session/:id/profileenables profiling for a session (must be called before/generate);GET /session/:id/profilereturns the collectedSessionProfileas JSON, 404 if never enabled.post_generateonly threads the registry through when the session was explicitly enabled — profiling is strictly opt-in.main.rs:NodeStategains aprofiling: ProfileRegistryfield and the new route.No new dependencies — built entirely on
sysinfoandtokio::process, both already direct dependencies (sysinfois already used the same way for the node's own memory metric inmain.rs's background updater).Rebase note
This branch was rebased onto
upstream/mainafter creation, which had gained an unrelated per-phase timeout watchdog (PhaseTimeouts, added as a new parameter torun_proof_generation) in the meantime — merged cleanly alongside theprofileparameter, since the two don't interact.Acceptance Criteria
docs/PROFILING.md(pprof's model doesn't apply to sampling an opaque external process)POST/GET /session/:id/profileTests
src/profiling.rs'stestsmodule:ProfileRegistryenable/is_enabled/get semantics (per-session isolation, idempotent re-enable that doesn't clear prior phases,Nonefor a never-enabled session), phases append rather than overwrite across the three phase names, and one#[cfg(unix)]test spawning a real short-livedsh -c "sleep 0.3"child to confirmsample_process_until_exitrecords a phase with non-zero duration once it exits.Verification
No local Rust toolchain is available in this environment (
link.exefails compilingproc-macro2/quotebuild scripts).cargo test -p mpc-nodecould not be run here. Verified by close manual review, including checking therun_profiledcall sites against the freshly-merged watchdog deadline checks to confirm no collision. Would appreciate CI/a reviewer confirming locally.Closes #244