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
Avoid bevy_reflect::List::iter wrapping in release mode #13271
Conversation
b2e7d86
to
da7ec0d
Compare
I think you're underestimating how long the test will take. use std::{hint::black_box, time::Instant};
fn next_if_some(num: u32, b: Option<bool>) -> u32 {
num + b.is_some() as u32
}
pub fn main() {
let mut val = 0;
let start = Instant::now();
for _ in 0..(u32::MAX) {
// Both black boxes are needed to so this doesn't get optimized to a no op
val = black_box(next_if_some(val, black_box(None)))
}
let elapsed = start.elapsed();
println!("Took {:?}", start.elapsed());
let mut projected = elapsed;
// Attempt to calculate how long the loop would take if we tested til usize::MAX (on 64 bit cpu)
for _ in 0..32 {
projected *= 2;
}
println!("Projected for u64 {:?}", projected)
} Running it on my Ryzen 7 7800x3d outputs:
7723814923.0624768 seconds is 244 years! |
This code won't handle vecs with zsts. One can construct a vector with usize::MAX elements today, like this: fn max_zst_vec() -> Vec<()> {
let mut data: Vec<()> = Vec::new();
// SAFETY: A zst vec has a capacity of usize::MAX
unsafe { data.set_len(usize::MAX) }
data
} Your code will:
|
Maybe I'm stupid but if you have I changed the test to use a zero size type vec and removed the loop though, let me know if you think it's okay. Edit: maybe I should stop force pushing, I see it looks a bit strange on the code review parts. Maybe someone can let me know the preferred way of working here |
You're not stupid. I was wrong. |
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.
simple, correct change. LGTM!
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.
Looks good! Thanks for writing the test as well!
It would be great if we could do this for the other iterator types as well, but that isn't a blocker for this PR.
crates/bevy_reflect/src/list.rs
Outdated
@@ -522,4 +524,23 @@ mod tests { | |||
assert_eq!(index, value); | |||
} | |||
} | |||
|
|||
#[cfg(not(debug_assertions))] |
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.
Does CI run tests in release mode? If not, it might be something we raise an issue for such that tests like these can be run.
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.
it doesn't...
@@ -374,7 +374,7 @@ impl<'a> Iterator for ArrayIter<'a> { | |||
#[inline] | |||
fn next(&mut self) -> Option<Self::Item> { | |||
let value = self.array.get(self.index); | |||
self.index += 1; | |||
self.index += value.is_some() as usize; |
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.
Awesome! Thanks for adding these as well!
Would also be able to duplicate that test for each of these changes as well?
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.
I couldnt test the overflow for most of them because I couldnt create an iterator with enough elements.
But the logic of not incrementing past the first None I can test, so I've added tests for that
Objective
Fixes #13230
Solution
Uses solution described in #13230
They mention a worry about adding a branch, but I'm not sure there is one.
This code
produces this assembly with opt-level 3
Testing
Added test from #13230, tagged it as ignore as it is only useful in release mode and very slow if you accidentally invoke it in debug mode.
Changelog
Iterationg of ListIter will no longer overflow and wrap around
Migration Guide