Skip to content

Add runner options to enable valgrind subprocess tracking - #493

Open
GuillaumeLagrange wants to merge 2 commits into
mainfrom
cod-2349-support-subprocesses-in-valgrind
Open

Add runner options to enable valgrind subprocess tracking#493
GuillaumeLagrange wants to merge 2 commits into
mainfrom
cod-2349-support-subprocesses-in-valgrind

Conversation

@GuillaumeLagrange

@GuillaumeLagrange GuillaumeLagrange commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Related: CodSpeedHQ/valgrind-codspeed#25

Not to be merged until valgrind-codspeed has been released and required version has been bumped

GuillaumeLagrange and others added 2 commits July 30, 2026 09:29
- Pass --separate-threads=yes to callgrind so each OS thread dumps its own cost
  sections (valgrind side fixed in COD-3196, parsed by the backend in COD-3197).
- Add --track-simulation-subprocesses (CODSPEED_TRACK_SIMULATION_SUBPROCESSES, off
  by default), which runs valgrind with --instr-atstart=inherit instead of =no. A
  process reached through an exec then picks up the instrumentation state of the
  image it replaced, so a benchmark that spawns subprocesses has their cost measured
  and attributed to it through the ppid/spawn-part dump headers. Children also
  inherit LD_PRELOAD and the benchmark URI env, so exec-harness children additionally
  dump under the benchmark URI directly.
- Drop the exec-harness guard that failed runs whose benchmark process spawned
  subprocesses under valgrind (TODO(COD-2163)): those runs now succeed either way,
  measuring the children when tracking is enabled and ignoring them otherwise.

Refs COD-2349
Co-Authored-By: Claude <noreply@anthropic.com>
This was a bandaid fix that is better fixed in other ways, since we
should now be able to do codpseed exec -m simulation -- cargo run
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from 5ce8ad4 to 9ab21d0 Compare July 30, 2026 07:29
Comment thread src/executor/valgrind/measure.rs Outdated
@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown

Greptile Summary

Adds opt-in Valgrind subprocess measurement support.

  • Introduces a shared CLI and environment option for subprocess tracking.
  • Propagates the option through orchestrator and executor configuration.
  • Adjusts Valgrind invocation and output handling for subprocess and thread measurements.
  • Removes the exec harness’s previous subprocess rejection.

Confidence Score: 4/5

The PR is not yet safe to merge because enabling subprocess tracking passes an unsupported Valgrind option value and prevents the benchmark from launching.

The subprocess-tracking path still emits --instr-atstart=inherit, while the pinned Valgrind parser treats this as a boolean option accepting only yes or no, so the newly exposed feature fails before benchmark execution.

Files Needing Attention: src/executor/valgrind/measure.rs

Important Files Changed

Filename Overview
src/cli/shared.rs Adds the shared opt-in CLI and environment configuration for simulation subprocess tracking.
src/executor/config.rs Propagates subprocess-tracking configuration from orchestration into each execution.
src/executor/valgrind/measure.rs Changes Valgrind argument construction to support subprocess and per-thread simulation output.
crates/exec-harness/src/analysis/mod.rs Removes the post-execution guard that rejected benchmarks spawning Valgrind-traced subprocesses.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A["CLI flag / environment variable"] --> B["OrchestratorConfig"]
  B --> C["ExecutorConfig"]
  C --> D["Valgrind argument construction"]
  D --> E["Instrument benchmark subprocesses"]
  E --> F["Combined simulation artifacts"]
Loading

Reviews (2): Last reviewed commit: "chore: stop skipping rustup wrapper for ..." | Re-trigger Greptile

@codspeed-hq

codspeed-hq Bot commented Jul 30, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 14 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing cod-2349-support-subprocesses-in-valgrind (5ce8ad4) with main (8b253c6)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants