Removed once_cell

[bushrat011899]

Objective

• Fixes Replace once_cell with std lib's implementation #8303

Solution

• Replaced 1 instance of OnceBox<T> with OnceLock<T> in NonGenericTypeCell

Notes

All changes are in the private side of Bevy, and appear to have no observable change in performance or compilation time. This is purely to reduce the quantity of direct dependencies in Bevy.

[james7132]

It's not clear this is 100% equivalent since the once_cell version allows racing to initialize, while the std implementation is unclear what its behavior is, or if it uses a lock underneath or not. These come with semantic and potential performance differences depending on the underlying implementation.

Since this seems to be a fairly foundational type within bevy_reflect, I'd defer to the reflection SMEs to make a final call on whether this ambiguity is acceptable or not.

[bushrat011899]

Since this seems to be a fairly foundational type within bevy_reflect, I'd defer to the reflection SMEs to make a final call on whether this ambiguity is acceptable or not.

Perfectly reasonable. Comparing OnceLock::get_or_init and OnceBox::get_or_init, the key difference is whether it races to completion (fastest thread wins), or blocks until completion (first thread wins).

Based on the handful of locations I can see the NonGenericTypeCell::get_or_set being used, they all appear to have "constant" parameters:

// Snip from bevy_asset/src/path.rs

impl Typed for AssetPath<'static> {
    fn type_info() -> &'static TypeInfo {
        static CELL: NonGenericTypeInfoCell = NonGenericTypeInfoCell::new();
        CELL.get_or_set(|| {
            let info = ValueInfo::new::<Self>();
            TypeInfo::Value(info)
        })
//     ^--------------------------------------^
//       This will always produce the same value
    }
}

So the value won't change moving from race to block behaviour, leaving timing as the only other possible change. Again, all the calls to NonGenericTypeCell::get_or_set have identical methods and parameters. As such, I think the blocking behaviour will perform slightly better than race, as only one copy of the initializer will ever be run, instead of multiple threads running it simultaneously. I don't think that performance change could ever be observed, since these functions are so short.

[SkiFire13]

Note that the race can happen only once. Moreover they are very likely to be initialized at the start of the program, when an App is created and the types are registered in the TypeRegistry. Thus there's likely no race anyway.

If anything, I would remove the Box. It was needed in OnceBox only because there's no way to race the initialization of arbitrary types, but it's very easy to do so when the type involved is represented as a Box. Thus if we don't need the race we can just remove the Box and save a bunch of allocations.

[bushrat011899]

If anything, I would remove the Box. It was needed in OnceBox only because there's no way to race the initialization of arbitrary types, but it's very easy to do so when the type involved is represented as a Box. Thus if we don't need the race we can just remove the Box and save a bunch of allocations.

Just got some time to test that, and removing the closure and Box in NonGenericTypeCell::get_or_set appears to make no difference, so I've removed them for the sake of simplicity. I don't have any benchmarking setup to measure a performance difference, but just playing some of the examples I couldn't notice anything different. Personally, I think removing it makes that code a tiny bit cleaner. Thanks for the suggestion!

[james7132]

Checking the usage patterns a bit more deeply, I'm inclined to agree that this shouldn't be too big of an issue outside of building the type registry. Still unfortunate that we need this at all since this does seem to be necessary only because there's no const constructor for TypeId or type_name.

[bushrat011899]

Still unfortunate that we need this at all since this does seem to be necessary only because there's no const constructor for TypeId or type_name.

Completely agree. I was looking at the TypePath trait just earlier thinking it would be great to make this all const. It seems a lot of code could start becoming const if we had access to a const type_name...