Skip to content

feat(memtrack): record per-benchmark pid in execution timestamps#458

Open
GuillaumeLagrange wants to merge 1 commit into
cod-1801-only-attach-to-used-libraries-in-memtrackfrom
cod-3010-improve-node-memory-benchmarks-stability
Open

feat(memtrack): record per-benchmark pid in execution timestamps#458
GuillaumeLagrange wants to merge 1 commit into
cod-1801-only-attach-to-used-libraries-in-memtrackfrom
cod-3010-improve-node-memory-benchmarks-stability

Conversation

@GuillaumeLagrange

Copy link
Copy Markdown
Contributor

Memory mode tracks the whole runner process tree, so between Start/StopProfiler the harness parent and sibling worker processes emit allocation events into every measured region. The CurrentBenchmark FIFO command already carries the pid that ran each benchmark; capture it alongside the uri in ExecutionTimestamps.bench_pid_by_ts (serde-default for artifacts written before this field) so the parser can scope events to the benchmarking process.

@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3010-improve-node-memory-benchmarks-stability branch from 09b7354 to 280ddf0 Compare July 17, 2026 09:56
@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown

Greptile Summary

This PR records benchmark PIDs alongside execution timestamps and adds fork events for memtrack process-tree filtering. The main changes are:

  • A new bench_pid_by_ts field on execution timestamp artifacts.
  • FIFO handling that stores each CurrentBenchmark PID with its timestamp.
  • A new memtrack Fork event type in the eBPF event schema.
  • Rust parsing and serialization support for fork events.

Confidence Score: 4/5

This is close, but the fork-event gap should be fixed before merging.

  • Fork edges can be omitted while PID tracking still records the child.
  • The new descendant filtering can then drop allocations from a tracked child process.
  • The schema and parser changes otherwise look consistent in the changed code.

crates/memtrack/src/ebpf/c/memtrack.bpf.c

Important Files Changed

Filename Overview
crates/memtrack/src/ebpf/c/memtrack.bpf.c Adds fork-event emission, but the profiling gate can omit edges needed for descendant filtering.
crates/memtrack/src/ebpf/c/event.h Adds the fork event type and parent-PID payload.
crates/memtrack/src/ebpf/events.rs Parses fork events into the shared memtrack event model.
crates/runner-shared/src/artifacts/execution_timestamps.rs Adds benchmark PID timestamps with defaulting for older artifacts.
crates/runner-shared/src/artifacts/memtrack/mod.rs Adds the shared Fork event variant and serialization coverage.
src/executor/shared/fifo.rs Stores the PID from CurrentBenchmark beside the benchmark URI timestamp.

Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/memtrack/src/ebpf/c/memtrack.bpf.c:121
**Fork Edges Can Disappear**

This gate can drop the process-tree edge that the new PID scoping depends on. The handler still adds the child to `tracked_pids` and `pids_ppid` before this branch, so a child forked while profiling is disabled can later emit allocation events after profiling starts, but the artifact never contains the matching `Fork` event. When the parser scopes events to the benchmark PID and descendants, that child has no recorded parent edge and its allocations can be treated as unrelated and dropped.

Reviews (5): Last reviewed commit: "feat(memtrack): record per-benchmark pid..." | Re-trigger Greptile

Comment thread src/executor/shared/fifo.rs
@codspeed-hq

codspeed-hq Bot commented Jul 17, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 17 untouched benchmarks


Comparing cod-3010-improve-node-memory-benchmarks-stability (663ead6) with cod-1801-only-attach-to-used-libraries-in-memtrack (4d3fbb0)

Open in CodSpeed

@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3010-improve-node-memory-benchmarks-stability branch 2 times, most recently from a5114a1 to 7d81688 Compare July 17, 2026 14:18
@GuillaumeLagrange
GuillaumeLagrange changed the base branch from main to cod-1801-only-attach-to-used-libraries-in-memtrack July 17, 2026 14:19
Memory mode tracks the whole runner process tree, so between
Start/StopProfiler the harness parent and sibling worker processes emit
allocation events into every measured region. The CurrentBenchmark FIFO
command already carries the pid that ran each benchmark; capture it
alongside the uri in ExecutionTimestamps.bench_pid_by_ts (serde-default
for artifacts written before this field) so the parser can scope events
to the benchmarking process.
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3010-improve-node-memory-benchmarks-stability branch from f3483ef to 663ead6 Compare July 17, 2026 16:26
@GuillaumeLagrange
GuillaumeLagrange marked this pull request as ready for review July 17, 2026 16:26
* like allocation events, so only forks inside a measured region reach
* the ring buffer. header.pid carries the child: current here is the
* parent, so we cannot use the SUBMIT_EVENT macro. */
if (is_enabled()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Fork Edges Can Disappear

This gate can drop the process-tree edge that the new PID scoping depends on. The handler still adds the child to tracked_pids and pids_ppid before this branch, so a child forked while profiling is disabled can later emit allocation events after profiling starts, but the artifact never contains the matching Fork event. When the parser scopes events to the benchmark PID and descendants, that child has no recorded parent edge and its allocations can be treated as unrelated and dropped.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/memtrack/src/ebpf/c/memtrack.bpf.c
Line: 121

Comment:
**Fork Edges Can Disappear**

This gate can drop the process-tree edge that the new PID scoping depends on. The handler still adds the child to `tracked_pids` and `pids_ppid` before this branch, so a child forked while profiling is disabled can later emit allocation events after profiling starts, but the artifact never contains the matching `Fork` event. When the parser scopes events to the benchmark PID and descendants, that child has no recorded parent edge and its allocations can be treated as unrelated and dropped.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

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.

1 participant