-
Notifications
You must be signed in to change notification settings - Fork 6
feat: profile memory of command #164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ca571b9 to
4bdc191
Compare
d168920 to
cffcf37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds memory profiling capability to the codebase through a new Memory executor that uses eBPF-based tracking via a custom heaptrack crate. The implementation allows tracking memory allocations (malloc, free, calloc, realloc, etc.) in benchmarked programs.
Key Changes:
- Introduces a new
MemoryExecutorthat uses eBPF uprobes to track memory allocations - Adds a
heaptrackcrate with BPF programs for monitoring malloc/free and related syscalls - Refactors FIFO communication handling into shared code to support multiple executor types
- Updates test snapshots to use Git LFS for large binary data
Reviewed changes
Copilot reviewed 52 out of 54 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/run/runner/memory/executor.rs |
Implements the new Memory executor using heaptrack IPC for control |
src/run/runner/shared/fifo.rs |
Extracts shared FIFO message handling logic for reuse across executors |
src/run/runner/wall_time/perf/mod.rs |
Refactors to use shared FIFO handling code |
crates/heaptrack/* |
New crate providing eBPF-based memory tracking with BPF programs and IPC |
src/run/uploader/upload.rs |
Adds Memory executor case (currently unimplemented) |
flake.nix |
Adds Nix development environment configuration |
.github/workflows/ci.yml |
Updates CI to install dependencies and run BPF tests separately |
| Snapshot files | Migrates large test snapshots to Git LFS |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
9aaf842 to
28a793b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI does not build, and the CI change commit scares me a bit, going forward not gonna lie
Just so I know, have you had the protobuf decision challenged ?
Could be good to have a small design document just to make sure the whole team is aligned behind the choice.
0e32eca to
9917752
Compare
b05d317 to
100f423
Compare
|
@GuillaumeLagrange As discussed, I updated the benchmark result types to use |
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool! Let me know if you want to discuss naming
863e36a to
5f87e90
Compare
a0c59b0 to
d761c18
Compare
aab7676 to
c47450e
Compare
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
olgtm
1c27363 to
524ff20
Compare
524ff20 to
3f08e89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 47 out of 48 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3f08e89 to
03c7db9
Compare
|
As discussed, I'll merge this now and we'll implement further improvements and fixes afterwards. |
Changes in this PR:
TODO:
Parse the backend responseskipped for now, we'll add this later