Use a RandomState HashBuilder for re-exported HashMap and HashSet#18691
Use a RandomState HashBuilder for re-exported HashMap and HashSet#18691alice-i-cecile wants to merge 4 commits intobevyengine:mainfrom
Conversation
| /// | ||
| /// To use an [`Interner<T>`], `T` must implement [`Internable`]. | ||
| pub struct Interner<T: ?Sized + 'static>(RwLock<HashSet<&'static T>>); | ||
| pub struct Interner<T: ?Sized + 'static>(RwLock<HashSet<&'static T, FixedHasher>>); |
There was a problem hiding this comment.
This is making this choice explicit. Deterministic hashing seems appropriate here.
|
|
||
| /// Shortcut for [`Entry`](hb::Entry) with [`FixedHasher`] as the default hashing provider. | ||
| pub type Entry<'a, T, S = FixedHasher> = hb::Entry<'a, T, S>; | ||
| pub use hb::{Entry, ExtractIf, HashSet, OccupiedEntry, VacantEntry}; |
There was a problem hiding this comment.
We can simply re-export now.
| (MeshVertexBufferLayoutRef, S::Key), | ||
| CachedRenderPipelineId, | ||
| FixedHasher, | ||
| RandomState, |
There was a problem hiding this comment.
I changed this to RandomState as it was a much less intrusive change: the alternative is adding complex generics to a ton of Entry callsites. Rendering people: does this choice matter?
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
This doesn't need a migration guide, as it's relative to 0.15, and the existing migration guides around crate path are already covered. |
|
Migration guide because it makes the iteration order no longer stable. If someone was relying on them (we merged
|
|
I prefer #18694: I want to completely avoid breaking changes here. |
# 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)`.
Objective
The changes to the Hash made in #15801 to the
BuildHasherresulted in serious migration problems and downgraded UX for users of Bevy's re-exported hashmaps.Fixes #18690. Closes bevyengine/bevy-website#2065, and closes bevyengine/bevy-website#2045, as this is no longer a user-facing migration. Once merged, we need to go in and remove the migration guide added as part of #15801.
Solution
This PR reverts that change while keeping the hashbrown version upgrade (and corresponding new improved hashing strategy).
Testing
I've added a regression test demonstrating that these methods can be called and produce the expected type. This fails before, as
HashMap::newetc are only implemented for one particular choice ofS: BuildHasher.Migration Guide
Bevy's default
HashMapandHashSettypes no longer have a deterministic initialization strategy. If you were relying on this, useFixedStateas theSgeneric on these types.