-
Notifications
You must be signed in to change notification settings - Fork 6
feat(memtrack): add zstd compression support #184
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
feat(memtrack): add zstd compression support #184
Conversation
1afc939 to
f58ee6c
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 PR adds zstd compression support to memtrack artifacts to reduce storage requirements. The implementation introduces streaming compression/decompression and refactors the event processing architecture to write events directly to disk during collection rather than accumulating them in memory.
Key changes:
- Introduces
MemtrackWriterwith zstd compression for streaming event serialization - Refactors
track_commandto use a two-stage threading model (drain thread → writer thread) for immediate disk writing - Centralizes filename generation logic in the
ArtifactExttrait
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/runner-shared/src/artifacts/mod.rs | Adds file_name() method to centralize filename generation for artifacts with optional PID |
| crates/runner-shared/src/artifacts/memtrack.rs | Introduces MemtrackWriter struct with zstd compression and updates decoder to decompress streams |
| crates/memtrack/src/main.rs | Refactors event processing to use two-stage threading (drain + writer) for streaming events to compressed disk files |
| crates/runner-shared/Cargo.toml | Adds zstd dependency version 0.13 |
| Cargo.lock | Updates lock file with zstd and its transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f3e451e to
2cb7cc4
Compare
CodSpeed Performance ReportCongrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
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 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| format!("{pid}.{}.msgpack", Self::name()) | ||
| } else { | ||
| format!("{}.msgpack", Self::name()) |
Copilot
AI
Jan 5, 2026
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.
The file extension .msgpack is misleading now that the files are compressed with zstd. Consider changing the extension to .msgpack.zst or .msgpack.zstd to accurately reflect that the files are compressed, which will help users understand the file format and ensure proper handling.
| format!("{pid}.{}.msgpack", Self::name()) | |
| } else { | |
| format!("{}.msgpack", Self::name()) | |
| format!("{pid}.{}.msgpack.zst", Self::name()) | |
| } else { | |
| format!("{}.msgpack.zst", Self::name()) |
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.
I agree that it's misleading, but I think it's not worth it changing the file extension. The data format is hidden in the MemtrackWriter and the MemtrackEventStream, so the consumer doesn't care about it.
2cb7cc4 to
35e5f3f
Compare
Changes in this PR: