Update hashbrown to 0.15#15801
Conversation
1f86e6e to
89ccf19
Compare
89ccf19 to
7fb8bb7
Compare
|
@clarfonthey looks like there's a formatting issue somewhere, could you try running the formatter and push again? |
|
Yeah, I was running into some difficulties fixing the lints but will try and fix this today or tomorrow. |
|
Rereading the changelog, I misunderstood what hashbrown did and they actually switched to a better hash that we should probably use instead. Gonna probably find a way to factor that in. |
9e73303 to
bba0dda
Compare
crates/bevy_ecs/src/bundle.rs
Outdated
| initialize_dynamic_bundle(bundle_infos, components, Vec::from(component_ids)); | ||
| self.dynamic_bundle_storages | ||
| .insert_unique_unchecked(id, storages); | ||
| // SAFETY: We know the ID is unique |
There was a problem hiding this comment.
I'm adding this comment to appease the lint, but I actually don't know if we do know if the ID is unique.
There was a problem hiding this comment.
I´ve checked the places where Bundles::init_dynamic_info was being used and most of them are internal facing, but I found EntityWorldMut::insert_by_ids, EntityWorldMut::retain and EntityWorldMut::clear which might be public facing I think?
HashMap::insert_unique_unchecked says:
/// This operation is safe if a key does not exist in the map.
///
/// However, if a key exists in the map already, the behavior is unspecified:
/// this operation may panic, loop forever, or any following operation with the map
/// may panic, loop forever or return arbitrary result.
So I'm unsure if we should say something like the caller ensures the uniqueness, and even add some notes in the EntityWorldMut methods if applicable? I guess I'm thinking out loud. 😅
There was a problem hiding this comment.
So, I'm honestly fine just changing this to insert and leaving it at that :p
There was a problem hiding this comment.
Okay, so, I double-checked and the bundle ID is always increasing here, and without concurrency, we don't need to worry about data races. So, I updated the comment to clarify it is actually unique and leaving it.
965855a to
ed0e28a
Compare
|
This should be ready for review, modulo whatever tiny nitpicks CI throws at me. Not sure if the migration guide is adequate: not sure how much we care about bevy_utils' public API, as far as like, whether people should be using it or not. |
ed0e28a to
0f2237a
Compare
|
What is better about foldhash vs ahash? Is it really worth taking the extra dependency? |
|
(I'll try and take a look at the failures soon) |
|
Ah, fun: the singular PR that got in the queue before this one didn't result in any merge conflicts, but it did add more uses of Those have to be removed now, since |
ca0e5f2 to
44a6b54
Compare
|
(Merge conflict should be resolved; ready to merge now.) |
44a6b54 to
71989d9
Compare
|
(kind of shocked that |
|
(Okay, this time I waited until CI passed. Should be ready to merge now. <3) |
|
Thanks for shepherding this across the finish line <3 |
|
So, I had no idea about these patches that were being run in CI, but they seem pretty brittle-- it would be much better (IMHO) to use |
71989d9 to
829d57e
Compare
|
I also have genuinely no idea why that patch failed on this PR. It appears to be referencing an earlier change where the patch should have broken, but it somehow did not trigger until now. And there were no recent changes to the CI job, either. It just makes absolutely no sense why this PR would be the one to trigger it, but at least, it did, and I fixed it. |
Updating dependencies; adopted version of bevyengine#15696. (Supercedes bevyengine#15696.) Long answer: hashbrown is no longer using ahash by default, meaning that we can't use the default-hasher methods with ahasher. So, we have to use the longer-winded versions instead. This takes the opportunity to also switch our default hasher as well, but without actually enabling the default-hasher feature for hashbrown, meaning that we'll be able to change our hasher more easily at the cost of all of these method calls being obnoxious forever. One large change from 0.15 is that `insert_unique_unchecked` is now `unsafe`, and for cases where unsafe code was denied at the crate level, I replaced it with `insert`. ## Migration Guide `bevy_utils` has updated its version of `hashbrown` to 0.15 and now defaults to `foldhash` instead of `ahash`. This means that if you've hard-coded your hasher to `bevy_utils::AHasher` or separately used the `ahash` crate in your code, you may need to switch to `foldhash` to ensure that everything works like it does in Bevy.
Updating dependencies; adopted version of bevyengine#15696. (Supercedes bevyengine#15696.) Long answer: hashbrown is no longer using ahash by default, meaning that we can't use the default-hasher methods with ahasher. So, we have to use the longer-winded versions instead. This takes the opportunity to also switch our default hasher as well, but without actually enabling the default-hasher feature for hashbrown, meaning that we'll be able to change our hasher more easily at the cost of all of these method calls being obnoxious forever. One large change from 0.15 is that `insert_unique_unchecked` is now `unsafe`, and for cases where unsafe code was denied at the crate level, I replaced it with `insert`. ## Migration Guide `bevy_utils` has updated its version of `hashbrown` to 0.15 and now defaults to `foldhash` instead of `ahash`. This means that if you've hard-coded your hasher to `bevy_utils::AHasher` or separately used the `ahash` crate in your code, you may need to switch to `foldhash` to ensure that everything works like it does in Bevy.
# Objective - Fixes #18690 - Closes [#2065](bevyengine/bevy-website#2065) - Alternative to #18691 The changes to the Hash made in #15801 to the [BuildHasher](https://doc.rust-lang.org/std/hash/trait.BuildHasher.html) resulted in serious migration problems and downgraded UX for users of Bevy's re-exported hashmaps. Once merged, we need to go in and remove the migration guide added as part of #15801. ## Solution - Newtype `HashMap` and `HashSet` instead of type aliases - Added `Deref/Mut` to allow accessing future `hashbrown` methods without maintenance from Bevy - Added bidirectional `From` implementations to provide escape hatch for API incompatibility - Added inlinable re-exports of all methods directly to Bevy's types. This ensures `HashMap::new()` works (since the `Deref` implementation wont cover these kinds of invocations). ## Testing - CI --- ## Migration Guide - If you relied on Bevy's `HashMap` and/or `HashSet` types to be identical to `hashbrown`, consider using `From` and `Into` to convert between the `hashbrown` and Bevy types as required. - If you relied on `hashbrown/serde` or `hashbrown/rayon` features, you may need to enable `bevy_platform_support/serialize` and/or `bevy_platform_support/rayon` respectively. --- ## Notes - Did not replicate the Rayon traits, users will need to rely on the `Deref/Mut` or `From` implementations for those methods. - Did not re-expose the `unsafe` methods from `hashbrown`. In most cases users will still have access via `Deref/Mut` anyway. - I have added `inline` to all methods as they are trivial wrappings of existing methods. - I chose to make `HashMap::new` and `HashSet::new` const, which is different to `hashbrown`. We can do this because we default to a fixed-state build-hasher. Mild ergonomic win over using `HashMap::with_hasher(FixedHasher)`.
# Objective - Fixes #18690 - Closes [#2065](bevyengine/bevy-website#2065) - Alternative to #18691 The changes to the Hash made in #15801 to the [BuildHasher](https://doc.rust-lang.org/std/hash/trait.BuildHasher.html) resulted in serious migration problems and downgraded UX for users of Bevy's re-exported hashmaps. Once merged, we need to go in and remove the migration guide added as part of #15801. ## Solution - Newtype `HashMap` and `HashSet` instead of type aliases - Added `Deref/Mut` to allow accessing future `hashbrown` methods without maintenance from Bevy - Added bidirectional `From` implementations to provide escape hatch for API incompatibility - Added inlinable re-exports of all methods directly to Bevy's types. This ensures `HashMap::new()` works (since the `Deref` implementation wont cover these kinds of invocations). ## Testing - CI --- ## Migration Guide - If you relied on Bevy's `HashMap` and/or `HashSet` types to be identical to `hashbrown`, consider using `From` and `Into` to convert between the `hashbrown` and Bevy types as required. - If you relied on `hashbrown/serde` or `hashbrown/rayon` features, you may need to enable `bevy_platform_support/serialize` and/or `bevy_platform_support/rayon` respectively. --- ## Notes - Did not replicate the Rayon traits, users will need to rely on the `Deref/Mut` or `From` implementations for those methods. - Did not re-expose the `unsafe` methods from `hashbrown`. In most cases users will still have access via `Deref/Mut` anyway. - I have added `inline` to all methods as they are trivial wrappings of existing methods. - I chose to make `HashMap::new` and `HashSet::new` const, which is different to `hashbrown`. We can do this because we default to a fixed-state build-hasher. Mild ergonomic win over using `HashMap::with_hasher(FixedHasher)`.

Updating dependencies; adopted version of #15696. (Supercedes #15696.)
Long answer: hashbrown is no longer using ahash by default, meaning that we can't use the default-hasher methods with ahasher. So, we have to use the longer-winded versions instead. This takes the opportunity to also switch our default hasher as well, but without actually enabling the default-hasher feature for hashbrown, meaning that we'll be able to change our hasher more easily at the cost of all of these method calls being obnoxious forever.
One large change from 0.15 is that
insert_unique_uncheckedis nowunsafe, and for cases where unsafe code was denied at the crate level, I replaced it withinsert.Migration Guide
bevy_utilshas updated its version ofhashbrownto 0.15 and now defaults tofoldhashinstead ofahash. This means that if you've hard-coded your hasher tobevy_utils::AHasheror separately used theahashcrate in your code, you may need to switch tofoldhashto ensure that everything works like it does in Bevy.