-
Notifications
You must be signed in to change notification settings - Fork 141
Enforce release builds for benchmarks and simplify command interface #741
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?
Conversation
This seems like a much more sane approach than the previous iteration.... |
@@ -14,6 +14,12 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
*/ | |||
|
|||
// Ensure benchmarks only run with release builds for meaningful performance measurements |
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.
Can we remove this compile error? I often run the benchmark with debug symbos when profiling
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.
Removed the compile-time error in commit 668f0da. Benchmarks can now be compiled with debug symbols for profiling purposes. The Justfile still enforces release builds by default, but you can use cargo bench
directly if you need debug symbols.
Justfile
Outdated
cargo bench --profile={{ if target == "debug" { "dev" } else { target } }} {{ if features =="" {''} else { "--features " + features } }} -- --verbose | ||
bench-ci baseline target="release" features="": | ||
@# Benchmarks should only be run with release builds for meaningful results | ||
@if [ "{{ target }}" != "release" ]; then echo "Error: Benchmarks can only be run with release builds. Use 'just bench-ci <baseline> release' instead."; exit 1; fi |
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.
If this shouldn't be configurable, the parameter should just be removed.
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.
Removed the target parameter from both bench and bench-ci commands in commit c9c9f31. Since the commands only accept release builds, the parameter was indeed unnecessary and confusing. The commands now have cleaner signatures: just bench [features]
and just bench-ci baseline [features]
.
docs/benchmarking-hyperlight.md
Outdated
Use `just bench [debug/release]` parameter to run benchmarks. Comparing local benchmarks results to github-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor [tag] ` to download and extract the GitHub release benchmarks to the correct place folder. You can then run `just bench-ci main` to compare to (and overwrite) the previous release benchmarks. Note that `main` is the name of the baselines stored in GitHub. | ||
Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to github-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor [tag] ` to download and extract the GitHub release benchmarks to the correct place folder. You can then run `just bench-ci main` to compare to (and overwrite) the previous release benchmarks. Note that `main` is the name of the baselines stored in GitHub. | ||
|
||
**Important**: Benchmarks can only be run with release builds to ensure meaningful performance measurements. Debug builds are not supported for benchmarking. |
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.
This note is no longer applicable.
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.
Updated the documentation in commit to reflect that debug builds are now supported for profiling purposes. The note now clarifies that while just bench
uses release builds by default, developers can use cargo bench
directly for debug symbols.
Signed-off-by: Simon Davies <[email protected]>
Co-authored-by: jsturtevant <[email protected]> Signed-off-by: Simon Davies <[email protected]>
…g symbols Co-authored-by: ludfjig <[email protected]> Signed-off-by: Simon Davies <[email protected]>
- Remove target parameter from bench and bench-ci commands since they only accept release builds - Update GitHub workflows to remove target parameter usage - Simplify command signatures based on @syntactically's feedback Co-authored-by: syntactically <[email protected]> Signed-off-by: Simon Davies <[email protected]>
…or profiling Co-authored-by: ludfjig <[email protected]> Signed-off-by: Simon Davies <[email protected]>
21560f8
to
3185a0e
Compare
This PR ensures that benchmarks run with release builds by default for meaningful performance measurements, while simplifying the command interface and supporting debug builds for profiling purposes.
Changes Made
Justfile Updates:
target
parameter frombench
andbench-ci
commands since they only accept release buildsjust bench [features]
andjust bench-ci baseline [features]
--profile=release
for consistent performance measurementsGitHub Workflows Updates:
Benchmarks.yml
anddep_rust.yml
to use simplified command signaturesBenchmark Code Changes:
Documentation:
docs/benchmarking-hyperlight.md
to clarify thatjust bench
uses release builds by defaultcargo bench
directly for profiling with debug symbolsBehavior
The changes maintain the same core behavior while providing a cleaner interface:
just bench
consistently uses release profile for meaningful resultscargo bench
directlyExamples
Backward Compatibility
All existing workflows continue to work without modification. The changes are purely simplifications that remove unnecessary complexity while preserving all functionality and adding profiling flexibility.
Fixes #638.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.