-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add memory profiling support #39
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
Merging this PR will degrade performance by 35.81%
Performance Changes
Comparing |
b158911 to
003a1bf
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 memory profiling support to the benchmarking framework by introducing a new "memory" mode alongside the existing "simulation" and "walltime" modes. The changes unify the preprocessor directives and add the necessary configuration and infrastructure for memory profiling.
Changes:
- Renamed
CODSPEED_SIMULATIONpreprocessor macro toCODSPEED_ANALYSISto better represent both simulation and memory profiling modes - Added "memory" as a new valid mode in CMake and Bazel build configurations
- Removed warmup execution logic from simulation mode to reduce flakiness
- Added memory benchmark examples demonstrating various allocation patterns
- Updated CI workflows to test the new memory profiling mode with memtrack CLI installation
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| google_benchmark/src/benchmark_runner.cc | Updated preprocessor directive from CODSPEED_SIMULATION to CODSPEED_ANALYSIS |
| google_benchmark/src/benchmark_api_internal.h | Updated preprocessor directives and removed warmup execution logic |
| google_benchmark/src/benchmark_api_internal.cc | Removed warmup repetition code from simulation mode |
| google_benchmark/src/benchmark.cc | Updated preprocessor directives and console output message |
| google_benchmark/include/benchmark/benchmark.h | Updated preprocessor directives throughout the State class |
| examples/google_benchmark_cmake/memory_bench.hpp | Added new memory profiling benchmark examples with RLE encoding/decoding and allocation patterns |
| examples/google_benchmark_cmake/main.cpp | Added include for new memory benchmark header |
| examples/google_benchmark_bazel/memory_bench.hpp | Added symlink to cmake memory benchmark file |
| core/instrument-hooks | Updated submodule commit reference |
| core/CMakeLists.txt | Added "memory" to allowed modes and updated preprocessor definitions |
| core/BUILD | Added memory_mode configuration and updated preprocessor defines |
| .github/workflows/ci.yml | Added memory mode to test matrix and memtrack CLI installation steps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@not-matthias the ci fails, can you re-request a review once fixed ? 🙏 |
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, just wanted input from @art049 regarding the removal of the warmup run in analysis mode
d1ba173 to
16f4e61
Compare
0247b66 to
ad31ee3
Compare
TODO: REmove the custom memtrack installation before merging