Skip to content

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

Open
GuillaumeLagrange wants to merge 6 commits into
mainfrom
cod-3010-improve-node-memory-benchmarks-stability
Open

feat(memtrack): record per-benchmark pid in execution timestamps#458
GuillaumeLagrange wants to merge 6 commits into
mainfrom
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.

…tcher

Instead of pre-attaching every allocator found on the system, a BPF
fentry on security_mmap_file signals the first mapping of each unknown
executable inode. A background worker stops the mapping processes,
classifies the file by its symbols, attaches probes, and resumes them.
This covers dlopen'd and statically linked allocators in spawned
children, and skips libraries the benchmark never loads.

The public API shrinks to Tracker (owns the attach worker) and Session
(owns the spawned child and its event pipeline); ring buffers are
polled through a generic RingBufferPoller with caller-supplied parsing.

Fixes COD-1801
On-demand attach supersedes the startup scan: delete the system-wide
library glob, the build-dir walk, and the CODSPEED_MEMTRACK_BINARIES
env var (the runner no longer resolves exec target binaries for it).

BREAKING CHANGE: CODSPEED_MEMTRACK_BINARIES is no longer read

Refs COD-1801
Distro libjemalloc is built without the je_ symbol prefix, so symbol
classification missed it and fell through to libc++ (jemalloc exports
operator new), leaving plain malloc/aligned_alloc unprobed. Match on
mallocx, which keeps its name in unprefixed builds and is unique to
jemalloc.
@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

@not-matthias
not-matthias force-pushed the cod-1801-only-attach-to-used-libraries-in-memtrack branch from 4d3fbb0 to a7e1666 Compare July 20, 2026 10:11
Base automatically changed from cod-1801-only-attach-to-used-libraries-in-memtrack to main July 20, 2026 13:12

@not-matthias not-matthias 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.

LGTM overall

* 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
Member

Choose a reason for hiding this comment

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

I dont think we should do this type of filtering here, because it's still useful to be able to track the full execution tree. We have the timestamp so we can do the processing in the backend

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.

e.g. for RSS tracking I also needed to track fork/exec/exit even when memtrack isnt enabled: 8ef53f4 (#453)

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