From a0b994dd6336d0c18c511a5cb5b3c7900693a469 Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Mon, 5 Feb 2024 18:05:15 +0100 Subject: [PATCH] Hash stability guarantees (#11690) # Objective We currently over/underpromise hash stability: - `HashMap`/`HashSet` use `BuildHasherDefault` instead of `RandomState`. As a result, the hash is stable within the same run. - [aHash isn't stable between devices (and versions)](https://github.com/tkaitchuck/ahash?tab=readme-ov-file#goals-and-non-goals), yet it's used for `StableHashMap`/`StableHashSet` - the specialized hashmaps are stable Interestingly, `StableHashMap`/`StableHashSet` aren't used by Bevy itself (anymore). ## Solution Add/fix documentation ## Alternatives For `StableHashMap`/`StableHashSet`: - remove them - revive #7107 --- ## Changelog - added iteration stability guarantees for different hashmaps --- crates/bevy_reflect/src/utility.rs | 8 ++--- crates/bevy_utils/src/lib.rs | 50 +++++++++++++++++++++++++----- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/crates/bevy_reflect/src/utility.rs b/crates/bevy_reflect/src/utility.rs index 75ad95b376a96..c658fa64c8c50 100644 --- a/crates/bevy_reflect/src/utility.rs +++ b/crates/bevy_reflect/src/utility.rs @@ -1,7 +1,7 @@ //! Helpers for working with Bevy reflection. use crate::TypeInfo; -use bevy_utils::{FixedState, StableHashMap}; +use bevy_utils::{FixedState, NoOpTypeIdHash, TypeIdMap}; use std::{ any::{Any, TypeId}, hash::BuildHasher, @@ -195,7 +195,7 @@ impl NonGenericTypeCell { /// ``` /// [`impl_type_path`]: crate::impl_type_path /// [`TypePath`]: crate::TypePath -pub struct GenericTypeCell(RwLock>); +pub struct GenericTypeCell(RwLock>); /// See [`GenericTypeCell`]. pub type GenericTypeInfoCell = GenericTypeCell; @@ -205,9 +205,7 @@ pub type GenericTypePathCell = GenericTypeCell; impl GenericTypeCell { /// Initialize a [`GenericTypeCell`] for generic types. pub const fn new() -> Self { - // Use `bevy_utils::StableHashMap` over `bevy_utils::HashMap` - // because `BuildHasherDefault` is unfortunately not const. - Self(RwLock::new(StableHashMap::with_hasher(FixedState))) + Self(RwLock::new(TypeIdMap::with_hasher(NoOpTypeIdHash))) } /// Returns a reference to the [`TypedProperty`] stored in the cell. diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index 49bcc8e09e0ed..840b7ae80c316 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -87,13 +87,17 @@ impl BuildHasher for FixedState { /// speed keyed hashing algorithm intended for use in in-memory hashmaps. /// /// aHash is designed for performance and is NOT cryptographically secure. +/// +/// Within the same execution of the program iteration order of different +/// `HashMap`s only depends on the order of insertions and deletions, +/// but it will not be stable between multiple executions of the program. pub type HashMap = hashbrown::HashMap>; /// A stable hash map implementing aHash, a high speed keyed hashing algorithm /// intended for use in in-memory hashmaps. /// -/// Unlike [`HashMap`] this has an iteration order that only depends on the order -/// of insertions and deletions and not a random source. +/// Unlike [`HashMap`] the iteration order stability extends between executions +/// using the same Bevy version on the same device. /// /// aHash is designed for performance and is NOT cryptographically secure. pub type StableHashMap = hashbrown::HashMap; @@ -102,13 +106,17 @@ pub type StableHashMap = hashbrown::HashMap; /// speed keyed hashing algorithm intended for use in in-memory hashmaps. /// /// aHash is designed for performance and is NOT cryptographically secure. +/// +/// Within the same execution of the program iteration order of different +/// `HashSet`s only depends on the order of insertions and deletions, +/// but it will not be stable between multiple executions of the program. pub type HashSet = hashbrown::HashSet>; /// A stable hash set implementing aHash, a high speed keyed hashing algorithm /// intended for use in in-memory hashmaps. /// -/// Unlike [`HashSet`] this has an iteration order that only depends on the order -/// of insertions and deletions and not a random source. +/// Unlike [`HashMap`] the iteration order stability extends between executions +/// using the same Bevy version on the same device. /// /// aHash is designed for performance and is NOT cryptographically secure. pub type StableHashSet = hashbrown::HashSet; @@ -224,6 +232,7 @@ impl Hasher for PassHasher { } /// A [`HashMap`] pre-configured to use [`Hashed`] keys and [`PassHash`] passthrough hashing. +/// Iteration order only depends on the order of insertions and deletions. pub type PreHashMap = hashbrown::HashMap, V, PassHash>; /// Extension methods intended to add functionality to [`PreHashMap`]. @@ -322,17 +331,30 @@ impl Hasher for EntityHasher { } /// A [`HashMap`] pre-configured to use [`EntityHash`] hashing. +/// Iteration order only depends on the order of insertions and deletions. pub type EntityHashMap = hashbrown::HashMap; /// A [`HashSet`] pre-configured to use [`EntityHash`] hashing. +/// Iteration order only depends on the order of insertions and deletions. pub type EntityHashSet = hashbrown::HashSet; /// A specialized hashmap type with Key of [`TypeId`] -pub type TypeIdMap = - hashbrown::HashMap>; +/// Iteration order only depends on the order of insertions and deletions. +pub type TypeIdMap = hashbrown::HashMap; -#[doc(hidden)] +/// [`BuildHasher`] for [`TypeId`]s. #[derive(Default)] +pub struct NoOpTypeIdHash; + +impl BuildHasher for NoOpTypeIdHash { + type Hasher = NoOpTypeIdHasher; + + fn build_hasher(&self) -> Self::Hasher { + NoOpTypeIdHasher(0) + } +} + +#[doc(hidden)] pub struct NoOpTypeIdHasher(u64); // TypeId already contains a high-quality hash, so skip re-hashing that hash. @@ -469,4 +491,18 @@ mod tests { std::hash::Hash::hash(&TypeId::of::<()>(), &mut Hasher); } + + #[test] + fn stable_hash_within_same_program_execution() { + let mut map_1 = HashMap::new(); + let mut map_2 = HashMap::new(); + for i in 1..10 { + map_1.insert(i, i); + map_2.insert(i, i); + } + assert_eq!( + map_1.iter().collect::>(), + map_2.iter().collect::>() + ); + } }