Avoid bevy_reflect::List::iter wrapping in release mode

[rmsthebest]

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

#[no_mangle]
pub fn next_if_some(num: i32, b: Option<bool>) -> i32 {
    num + b.is_some() as i32
}

produces this assembly with opt-level 3

next_if_some:
        xor     eax, eax
        cmp     sil, 2
        setne   al
        add     eax, edi
        ret

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

[Brezak]

I think you're underestimating how long the test will take.
I've written some code that should just test the increment.

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:

Took 1.7983411s
Projected for u64 7723814923.0624768s

7723814923.0624768 seconds is 244 years!
The loop in the test might get optimized out, but I don't know if we can rely on this.

[Brezak]

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:

• Iterate to the last element.
• Get the last element.
• Increment the index.
• The index being usize::MAX overflows into 0.
• Iterator loops forever!

[rmsthebest]

Maybe I'm stupid but if you have usize::MAX capacity the last element index is usize::MAX-1?

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

[Brezak]

Maybe I'm stupid but if you have usize::MAX capacity the last element index is usize::MAX-1?

You're not stupid. I was wrong.

[soqb]

simple, correct change. LGTM!

[MrGVSV]

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.

[MrGVSV]

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.

[soqb]

it doesn't...

[MrGVSV]

Awesome! Thanks for adding these as well!

Would also be able to duplicate that test for each of these changes as well?

[rmsthebest]

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