-
Notifications
You must be signed in to change notification settings - Fork 20
feat: add support for analysis mode #152
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
base: main
Are you sure you want to change the base?
feat: add support for analysis mode #152
Conversation
CodSpeed Performance ReportMerging #152 will degrade performances by 10%Comparing Summary
Benchmarks breakdown
|
8638bef to
5bab4b1
Compare
5bab4b1 to
f2e74d7
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 support for a new "analysis" measurement mode to the CodSpeed benchmarking framework. The analysis mode enables instrumentation similar to simulation mode and integrates with instrument hooks for tracking benchmark execution.
Key changes:
- Introduced a new
Analysisvariant to theMeasurementModeenum - Integrated
InstrumentHooksfor tracking benchmark lifecycle events - Removed the deprecated
is_instrumented()function andRunningOnValgrindenum variant
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/divan_compat/examples/benches/alloc.rs | New benchmark file for testing memory allocation scenarios |
| crates/divan_compat/examples/Cargo.toml | Added the new alloc benchmark to the build configuration |
| crates/codspeed/src/request/mod.rs | Removed unused RunningOnValgrind client request variant |
| crates/codspeed/src/measurement.rs | Removed deprecated is_instrumented() function |
| crates/codspeed/src/codspeed.rs | Integrated InstrumentHooks for benchmark lifecycle tracking |
| crates/cargo-codspeed/src/measurement_mode.rs | Added Analysis variant to measurement modes |
| crates/cargo-codspeed/src/build.rs | Extended codspeed cfg flag to apply to analysis mode |
| .github/workflows/ci.yml | Added CI workflow for testing memory analysis integration |
| crates/codspeed/instrument-hooks | Updated submodule commit reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f2e74d7 to
435a805
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
| # runner-branch: cod-1670-runner-profile-memory-of-command | ||
| # run: cargo codspeed run | ||
| # mode: memory | ||
| # token: ${{ secrets.CODSPEED_TOKEN }} |
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.
We should be able to remove secrets now with OIDC
| token: ${{ secrets.CODSPEED_TOKEN }} | ||
|
|
||
|
|
||
| compat-integration-test-memory: |
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.
We can have multi-dimensional matrices, as well as some dimensions precising two things
We could combine everything in one big matrix, one dimension being the library we are testing, the other one the mode and the runner it runs on like here https://github.com/CodSpeedHQ/codspeed-cpp/blob/f23b68b27f8543f88f95ff0bdc46785733dee061/.github/workflows/ci.yml#L57-L62
WDYT ?
We could conditionnallly remove the .cargo/config.toml, as well as conditionnally skip the uploading part for memory (for now)
| impl CodSpeed { | ||
| pub fn new() -> Self { | ||
| let is_instrumented = measurement::is_instrumented(); | ||
| let ih = InstrumentHooks::instance(); |
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.
not a fan of abbreviations overall, but here it does not bring that much, how about hooks ? or hooks_instance or instrument_hooks_instance ?
| pub fn start_benchmark(&mut self, name: &str) { | ||
| self.current_benchmark = CString::new(name).expect("CString::new failed"); | ||
| let _ = InstrumentHooks::instance().start_benchmark(); | ||
| measurement::start(); |
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.
will that not confuse valgrind to keep both this and the start_benchmark call when built in simulation ?
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.
It might be the time to say goodbye to the inline assembly and make a major out of this
|
|
||
| #[inline(always)] | ||
| pub fn end_benchmark(&mut self) { | ||
| measurement::stop(&self.current_benchmark); |
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.
same remark here
| "-Cstrip=none".to_owned(), | ||
| ]; | ||
|
|
||
| // Add the codspeed cfg flag if simulation mode is enabled |
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.
Update comment, or create a function that takes measurement_mode and outputs a bool, named should_build_benchmark_target_to_run_only_once or something like this, to convey the intention
No description provided.