Skip to content

Use a RandomState HashBuilder for re-exported HashMap and HashSet #18691

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Use a RandomState HashBuilder for re-exported HashMap and HashSet #18691

wants to merge 4 commits into from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Apr 2, 2025 •
edited
Loading

Objective

The changes to the Hash made in #15801 to the BuildHasher resulted 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::new etc are only implemented for one particular choice of S: BuildHasher.

Migration Guide

Bevy's default HashMap and HashSet types no longer have a deterministic initialization strategy. If you were relying on this, use FixedState as the S generic on these types.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use P-Regression Functionality that used to work but no longer does. Add a test for this! A-Utils Utility functions and types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 2, 2025
alice-i-cecile
alice-i-cecile commented Apr 2, 2025
View reviewed changes
crates/bevy_ecs/src/intern.rs
@@ -123,7 +123,7 @@ impl Internable for str {
/// The implementation ensures that two equal values return two equal [`Interned<T>`] values.
///
/// 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>>);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making this choice explicit. Deterministic hashing seems appropriate here.

alice-i-cecile
alice-i-cecile commented Apr 2, 2025
View reviewed changes
crates/bevy_platform_support/src/collections.rs

/// 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};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simply re-export now.

alice-i-cecile
alice-i-cecile commented Apr 2, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile commented Apr 2, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile commented Apr 2, 2025
View reviewed changes
crates/bevy_render/src/render_resource/pipeline_specializer.rs
@@ -138,7 +138,7 @@ impl<S: SpecializedMeshPipeline> SpecializedMeshPipelines<S> {
entry: VacantEntry<
(MeshVertexBufferLayoutRef, S::Key),
CachedRenderPipelineId,
FixedHasher,
RandomState,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@SpecificProtagonist SpecificProtagonist added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 2, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 2, 2025

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.

@alice-i-cecile alice-i-cecile removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 2, 2025
@alice-i-cecile
Copy link
Member Author

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.

@SpecificProtagonist
Copy link
Contributor

SpecificProtagonist commented Apr 2, 2025 •
edited
Loading

Migration guide because it makes the iteration order no longer stable. If someone was relying on them (we merged StableHashMap/StableHashSet into HashMap/HashSet), we're breaking their code. From 0.15:

Within the same execution of the program iteration order of different HashMaps only depends on the order of insertions and deletions […]

@bushrat011899 bushrat011899 mentioned this pull request Apr 3, 2025
Merged
@alice-i-cecile
Copy link
Member Author

I prefer #18694: I want to completely avoid breaking changes here.

github-merge-queue bot pushed a commit that referenced this pull request Apr 6, 2025
@bushrat011899
Newtype hashbrown (#18694)
7892f9e 
# 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)`.
mockersf pushed a commit that referenced this pull request Apr 8, 2025
@bushrat011899 @mockersf
Newtype hashbrown (#18694)
f3f4e80 
# 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)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Victoronz Victoronz Victoronz left review comments

@bushrat011899 bushrat011899 Awaiting requested review from bushrat011899

Assignees
No one assigned
Labels
A-Utils Utility functions and types C-Usability A targeted quality-of-life change that makes Bevy easier to use P-Regression Functionality that used to work but no longer does. Add a test for this! S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Milestone
No milestone
3 participants
@alice-i-cecile @SpecificProtagonist @Victoronz