feat(memtrack): record per-benchmark pid in execution timestamps#458
feat(memtrack): record per-benchmark pid in execution timestamps#458GuillaumeLagrange wants to merge 6 commits into
Conversation
…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.
09b7354 to
280ddf0
Compare
Greptile SummaryThis PR records benchmark PIDs alongside execution timestamps and adds fork events for memtrack process-tree filtering. The main changes are:
Confidence Score: 4/5This is close, but the fork-event gap should be fixed before merging.
crates/memtrack/src/ebpf/c/memtrack.bpf.c Important Files Changed
Prompt To Fix All With AIFix 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 |
Merging this PR will not alter performance
|
a5114a1 to
7d81688
Compare
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.
f3483ef to
663ead6
Compare
| * 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()) { |
There was a problem hiding this comment.
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.4d3fbb0 to
a7e1666
Compare
| * 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()) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
e.g. for RSS tracking I also needed to track fork/exec/exit even when memtrack isnt enabled: 8ef53f4 (#453)
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.