Skip to content

[COD-2349] feat(callgrind): track subprocesses across fork and exec - #25

Open
GuillaumeLagrange wants to merge 3 commits into
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-osfrom
cod-2349-support-subprocesses-in-valgrind
Open

[COD-2349] feat(callgrind): track subprocesses across fork and exec#25
GuillaumeLagrange wants to merge 3 commits into
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-osfrom
cod-2349-support-subprocesses-in-valgrind

Conversation

@GuillaumeLagrange

Copy link
Copy Markdown

Track subprocesses spawned by a benchmark so they can be measured and
attributed back to the benchmark that spawned them, across both fork and
exec.

Two independent pieces, one per commit:

1. Forward instrumentation state across a traced exec. A process spawned
via exec under --trace-children=yes gets a fresh valgrind whose
instrumentation state resets to --instr-atstart, so a benchmark reached
through an exec chain (e.g. cargo run) was measured from the wrong point. A
plain fork inherits the state with the address space, but an exec does not.
This adds a VG_(needs_child_exec_args) tool need: the core asks the tool for
extra valgrind arguments while building the child's argv at exec time and
appends them after VG_(args_for_valgrind) (later options win). Callgrind uses
it to forward --instr-atstart=<current state>. Wired into the exec argv
construction on Linux (generic), Darwin, and Solaris.

2. Record spawned subprocesses per dump part. When a benchmark spawns a
subprocess, the profile had no way to attribute that subprocess to the exact
benchmark (dump part) that spawned it. On fork the parent now records the new
child pid against the part currently being measured, and each part's header
lists the children spawned during it as desc: Spawned pid: lines (a desc
field so kcachegrind and callgrind_annotate ignore it). A fork child restarts
its part counter at 1 and drops the inherited edges so it only reports children
it spawns itself, and zeroes cost so work before the fork stays attributed to
the parent.

To carry the child pid to the fork hook, VG_(atfork)'s parent callback now
takes the pid of the just-created child; the pre/child callbacks are unchanged.
All fork/clone sites (Linux, FreeBSD, Solaris, generic) pass it through. The
child hook also re-stamps the in-flight fork syscall's start time from the
child's clocks, since the thread CPU clock restarts in the child and the exit
delta would otherwise underflow.

The spawn edge is recorded parent→child rather than child→parent: at fork time
the parent already knows both the child pid and its own live part, so no spawn
identity needs to cross the exec boundary — only the instrumentation state
does. This is a change from the earlier squashed design that emitted
desc: Spawned by: <pid>:<part> from the child; see the COD-2349 discussion for
the full rationale.

Stacked on top of #24 (COD-3196, per-thread dump support), which this builds
on for the retired-thread handling in the fork/exec paths.

Behavior verified with fork/exec integration tests in the valgrind-helpers repo
(subprocess measured and recorded across fork, across a traced exec, through a
deep fork tree, and one child recorded per part across a two-part run).

Closes COD-2349

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR tracks benchmark subprocesses across fork and exec. The main changes are:

  • Forwards Callgrind instrumentation state across traced execs.
  • Records spawned child PIDs against the active dump part.
  • Preserves and flushes retired-thread costs in per-thread dumps.
  • Passes child PIDs through parent fork callbacks on supported platforms.
  • Discards spawn records after their associated part is written.

Confidence Score: 5/5

This looks safe to merge.

  • The updated cleanup removes records only for the part that has finished dumping.
  • Empty-cost parts still write a section before their spawn records are discarded.
  • Output-open failures stop the path before cleanup, preserving records that were not emitted.

Important Files Changed

Filename Overview
callgrind/dump.c Writes subprocess metadata per dump part and removes emitted records to prevent unbounded retention.
callgrind/main.c Tracks forked children, resets child state, and forwards instrumentation state across execs.
callgrind/threads.c Retains exited-thread data until it can be included in per-thread dumps.
coregrind/m_libcproc.c Extends parent fork callbacks with the newly created child PID.

Reviews (5): Last reviewed commit: "feat(callgrind): inherit instrumentation..." | Re-trigger Greptile

Comment thread callgrind/dump.c Outdated
@codspeed-hq

codspeed-hq Bot commented Jul 22, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 84 untouched benchmarks
⏩ 60 skipped benchmarks1


Comparing cod-2349-support-subprocesses-in-valgrind (4d05dec) with cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os (febc434)2

Open in CodSpeed

Footnotes

  1. 60 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.

  2. No successful run was found on cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os (448b7d0) during the generation of this report, so 481509e was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from ce72315 to 69b355e Compare July 22, 2026 21:23
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from 69b355e to 002990c Compare July 22, 2026 21:56
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from 002990c to ec038a4 Compare July 27, 2026 12:27

@art049 art049 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed it would be simpler to have a new inherit mode that would resolve a file owned by the parent process declaring if instrumentation is enabled or not.
It will make the overall change in the logic of the codebase more isolated and easier to maintain.

@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch 2 times, most recently from aece00f to 4d05dec Compare July 28, 2026 14:37
When a benchmark spawns a subprocess, the resulting profile had no way to
attribute that subprocess back to the exact benchmark (dump part) that
spawned it, so the spawn tree could not be rebuilt.

Track fork edges and emit them in the dump: on fork the parent records
the new child pid against the part currently being measured, and each
part's header lists the children spawned during it as "desc: Spawned
pid:" lines (a desc field so kcachegrind and callgrind_annotate ignore
it). A fork child restarts its part counter at 1 and drops the inherited
edges so it only reports children it spawns itself, and zeroes cost so
work before the fork stays attributed to the parent.

To carry the child pid to the fork hook, VG_(atfork)'s parent callback
now takes the pid of the just-created child; the pre/child callbacks are
unchanged. All fork/clone sites (Linux, FreeBSD, Solaris, generic) pass
it through. The child hook also re-stamps the in-flight fork syscall's
start time from the child's clocks, since the thread CPU clock restarts
in the child and the exit delta would otherwise underflow.

Closes COD-2349
A process spawned via exec under --trace-children=yes gets a fresh
valgrind whose instrumentation state resets to --instr-atstart, so a
benchmark reached through an exec chain (e.g. `cargo run`) was measured
from the wrong point. A plain fork inherits the state with the address
space, but an exec does not.

Add --instr-atstart=inherit: like "no", except each process advertises
its current instrumentation state by keeping
<tmpdir>/callgrind-instr-<pid> in existence while enabled, and a fresh
valgrind adopts the state advertised for its own PID. An exec keeps the
PID, so the file maintained by the pre-exec image hands the state over;
a fork child inherits the state with the address space and republishes
it under its new PID. The file stores the process start time (stable
across exec), so a file left behind by a killed process is rejected
when its PID is reused.

Closes COD-2349
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-2349-support-subprocesses-in-valgrind branch from 4d05dec to c6a3e0b Compare July 28, 2026 14:44
@codspeed-hq

codspeed-hq Bot commented Jul 28, 2026

Copy link
Copy Markdown

Unable to generate the flame graphs

The performance report has correctly been generated, but there was an internal error while generating the flame graphs for this run. We're working on fixing the issue. Feel free to contact us on Discord or at support@codspeed.io if the issue persists.

@GuillaumeLagrange
GuillaumeLagrange requested a review from art049 July 29, 2026 08:00
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