feat(memtrack): record per-benchmark pid in execution timestamps#458
Conversation
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.
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.