Skip to content
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

Fix "Unrecognized Option" error when using Criterion-specific arguments in benchmarks #17222

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Jan 7, 2025

Objective

Solution

  • Disable the default libtest benchmark harness for the library crate, as per the Criterion book.

Testing

  • cargo bench -p benches -- --save-baseline before
    • You don't need to run the entire benchmarks, just make sure that they start without any errors. :)

@BD103 BD103 added C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy C-Benchmarks Stress tests and benchmarks used to measure how fast things are A-Cross-Cutting Impacts the entire engine S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2025
@BD103 BD103 requested a review from hymm January 7, 2025 16:13
@BenjaminBrienen
Copy link
Contributor

PS C:\Users\BenjaminBrienen\source\bevy> git status          
On branch fix-bench-unrecognized-option
Your branch is up to date with 'BD103/fix-bench-unrecognized-option'.

nothing to commit, working tree clean
PS C:\Users\BenjaminBrienen\source\bevy> cargo bench -- --save-baseline before
warning: unused import: `warn`
 --> crates\bevy_render\src\extract_resource.rs:7:22
  |
7 | use tracing::{error, warn};
  |                      ^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: `bevy_render` (lib) generated 1 warning (run `cargo fix --lib -p bevy_render` to apply 1 suggestion)
    Finished `bench` profile [optimized] target(s) in 0.41s
     Running unittests src\lib.rs (target\release\deps\bevy-d792cd0559babc77.exe)
error: Unrecognized option: 'save-baseline'
error: bench failed, to rerun pass `--lib`

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2025
@BD103
Copy link
Member Author

BD103 commented Jan 7, 2025

Looks like the warning if because tracing::error! is only used behind #[cfg(debug_assertions)], but the benchmarks are compiled with release mode. I'm working on a fix now!

Benchmarks and their dependencies are compiled with release mode. In the affected code, `warn!()` was only used when debug assertions were toggled. Instead of gating the `warn!()` import specifically, I chose to fully qualify the macro. (I also made the `error!()` call qualified, for consistency.)
@BD103 BD103 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 7, 2025
@BenjaminBrienen
Copy link
Contributor

PS C:\Users\BenjaminBrienen\source\bevy> gh pr checkout 17222
Already on 'fix-bench-unrecognized-option'
Your branch is up to date with 'BD103/fix-bench-unrecognized-option'.
Already up to date.
PS C:\Users\BenjaminBrienen\source\bevy> cargo bench -- --save-baseline before
    Finished `bench` profile [optimized] target(s) in 0.60s
     Running unittests src\lib.rs (target\release\deps\bevy-d792cd0559babc77.exe)
error: Unrecognized option: 'save-baseline'
error: bench failed, to rerun pass `--lib`

@BD103 still not recognizing the option

@BD103
Copy link
Member Author

BD103 commented Jan 7, 2025

Ah, I know! You need to cd benches first, since the benchmarks are associated with their own crate. The command that you should run is cargo bench -p benches -- --save-baseline before, for brevity. :)

Edit: I updated the original PR description with the better version of the command.

@BenjaminBrienen
Copy link
Contributor

Oh yeah! Oops, lol. Stand by while I try benching the benches.

@BenjaminBrienen
Copy link
Contributor

PS C:\Users\BenjaminBrienen\source\bevy> cargo bench -p benches -- --save-baseline before
    Finished `bench` profile [optimized] target(s) in 0.44s
     Running benches/bevy_ecs/main.rs (target\release\deps\ecs-0f0bbf4f3c69bf77.exe)
Gnuplot not found, using plotters backend
Benchmarking all_added_detection/5000_entities_ecs::change_detection::Table: Collecting 100 samples in estimated 4.3560 s (30k iterations)

much better!

@hymm
Copy link
Contributor

hymm commented Jan 7, 2025

Tried this, but it's still not saving the baselines. There's no benches/target/criterion folder and running critcmp --baselines doesn't show any either. Are they saving for you? Maybe it's a problem with my setup.

@BD103
Copy link
Member Author

BD103 commented Jan 7, 2025

There's no benches/target/criterion folder [...]

The benchmarks were recently added to the Cargo workspace, so it would be target/criterion, not benches/target/criterion.

What error message are you getting?

@hymm
Copy link
Contributor

hymm commented Jan 7, 2025

I didn't know that the benches were added to the workspace. I was running critcmp from the benches folder, so that was the problem. Works for me when I run it from the base folder.

@hymm hymm added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2025
@mockersf mockersf added this pull request to the merge queue Jan 8, 2025
Merged via the queue into bevyengine:main with commit 020d082 Jan 8, 2025
34 checks passed
@BD103 BD103 deleted the fix-bench-unrecognized-option branch January 8, 2025 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Benchmarks Stress tests and benchmarks used to measure how fast things are C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't save baselines when benchmarking
4 participants