Migrate reflection benchmarks to new naming system #16986
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added `black_box()` to places where random arguments should be simulated. See <https://doc.rust-lang.org/nightly/std/hint/fn.black_box.html#how-to-use-this> for more on this. - Moved cloning benchmarks to be above overloading benchmarks. - Renamed `overload()`'s `add()` to `simple()` to complement `complex()`. - Split `overload()` into `with_overload()` and `call_overload()`. - Switched `std` import to `core`. - Benchmarks will likely never be `#![no_std]`, but it's best to be consistent with the rest of the project.
Now benchmarks in this module can call `create_group()` instead of manually setting the warm-up and measurement time. I also set the plot scale to be logarithmic, since it looks better with our current sizes.
I also repeat what I did in e69f75e here by defining a `create_group()` function that sets common benchmark configuration.
That should be 316, not 3160 🤦 I do wonder how long that's been there, though! :)
- Use `BenchmarkId::from_parameter()` instead of repeating the benchmark name. - Change it so we only assert that the parse is successful during tests, not the actual benchmark.
Also extract the `create_group()` function.
There's definitely quite a few misconceptions about this function, and they can only be disproven by Compiler Explorer / Godolt. I try to keep a few things in mind: 1. Constant inputs should always be wrapped. 2. The value returned by the benchmark does not need to be wrapped because Criterion does it for you. 3. The input from `iter_batched()` does not need to be wrapped because Criterion does it for you.
It was unused, which is a good thing! Yippee!
BD103
added
C-Code-Quality
I'm not all that happy with making `criterion` a normal dependency, but in reality no one is going to build the `benches` crate who doesn't want it as well.
alice-i-cecile
approved these changes
Dec 26, 2024
BenjaminBrienen
approved these changes
Dec 26, 2024
alice-i-cecile
added
S-Ready-For-Final-Review
ecoskey
pushed a commit
to ecoskey/bevy
that referenced
this pull request
Jan 6, 2025
# Objective - Please see bevyengine#16647 for the full reasoning behind this change. ## Solution - Create the `bench!` macro, which generates the name of the benchmark at compile time. Migrating is a single line change, and it will automatically update if you move the benchmark to a different module: ```diff + use benches::bench; fn my_benchmark(c: &mut Criterion) { - c.bench_function("my_benchmark", |b| {}); + c.bench_function(bench!("my_benchmark"), |b| {}); } ``` - Migrate all reflection benchmarks to use `bench!`. - Fix a few places where `black_box()` or Criterion is misused. ## Testing ```sh cd benches # Will take a long time! cargo bench --bench reflect # List out the names of all reflection benchmarks, to ensure I didn't miss anything. cargo bench --bench reflect -- --list # Check for linter warnings. cargo clippy --bench reflect # Run each benchmark once. cargo test --bench reflect ```
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 4, 2025
# Objective - Please see bevyengine#16647 for the full reasoning behind this change. ## Solution - Create the `bench!` macro, which generates the name of the benchmark at compile time. Migrating is a single line change, and it will automatically update if you move the benchmark to a different module: ```diff + use benches::bench; fn my_benchmark(c: &mut Criterion) { - c.bench_function("my_benchmark", |b| {}); + c.bench_function(bench!("my_benchmark"), |b| {}); } ``` - Migrate all reflection benchmarks to use `bench!`. - Fix a few places where `black_box()` or Criterion is misused. ## Testing ```sh cd benches # Will take a long time! cargo bench --bench reflect # List out the names of all reflection benchmarks, to ensure I didn't miss anything. cargo bench --bench reflect -- --list # Check for linter warnings. cargo clippy --bench reflect # Run each benchmark once. cargo test --bench reflect ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Solution
Create the
bench!macro, which generates the name of the benchmark at compile time.Migrating is a single line change, and it will automatically update if you move the benchmark to a different module:
Migrate all reflection benchmarks to use
bench!.Fix a few places where
black_box()or Criterion is misused.Testing