Skip to content

Add black_box to some benchmarks that need it#23692

Open
nishantc1527 wants to merge 1 commit intobevyengine:mainfrom
nishantc1527:main
Open

Add black_box to some benchmarks that need it#23692
nishantc1527 wants to merge 1 commit intobevyengine:mainfrom
nishantc1527:main

Conversation

@nishantc1527
Copy link
Copy Markdown

Objective

The black_bxo function in Criterion is essential to make consistent benchmarks because LLVM likes to optimize things out randomly. It's a very weird and confusing process, it takes into account the size of the overall function and some random other stuff that's impossible to predict. So even though benchmarks work now, they may not work in the future. I wasn't able to reproduce any of this in the current state, but that's because it's so hard to predict LLVM.

Fixes #7156

Solution

I looked across the benches crate and found a few places that needed black_box. I didn't do a thorough check, I just kind of skimmed the files to look for things that stood out (constant numbers are usually a good tell, as well as unused variables).

Testing

It's a very hard thing to test because it's hard to reproduce. But looking at the docs (https://doc.rust-lang.org/beta/std/hint/fn.black_box.html) it's clear that every place I put it is necessary.


@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy C-Code-Quality A section of code that is hard to understand or change C-Benchmarks Stress tests and benchmarks used to measure how fast things are S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it A-Cross-Cutting Impacts the entire engine S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it D-Trivial Nice and easy! A great choice to get started with Bevy labels Apr 6, 2026
@alice-i-cecile alice-i-cecile requested a review from mockersf April 6, 2026 18:55
@alice-i-cecile
Copy link
Copy Markdown
Member

@mockersf I'm in favor of this, but giving you a heads-up because this will introduce spurious changes in the benchmarks.

@hymm
Copy link
Copy Markdown
Contributor

hymm commented Apr 7, 2026

Could you run before/after on the benchmarks that have changed? That way we can know what to expect in CI if there are any measurable changes.

@alice-i-cecile alice-i-cecile added S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 7, 2026
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-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help

Projects

None yet

Development

Successfully merging this pull request may close these issues.

May miss black_box calls in benches

3 participants