diff --git a/crates/iddqd/src/bi_hash_map/imp.rs b/crates/iddqd/src/bi_hash_map/imp.rs index 69976b6..00d267c 100644 --- a/crates/iddqd/src/bi_hash_map/imp.rs +++ b/crates/iddqd/src/bi_hash_map/imp.rs @@ -758,8 +758,14 @@ impl BiHashMap { /// ``` pub fn reserve(&mut self, additional: usize) { self.items.reserve(additional); - self.tables.k1_to_item.reserve(additional); - self.tables.k2_to_item.reserve(additional); + let items = &self.items; + let state = &self.tables.state; + self.tables + .k1_to_item + .reserve(additional, |ix| state.hash_one(items[*ix].key1())); + self.tables + .k2_to_item + .reserve(additional, |ix| state.hash_one(items[*ix].key2())); } /// Tries to reserve capacity for at least `additional` more elements to be @@ -815,13 +821,15 @@ impl BiHashMap { self.items .try_reserve(additional) .map_err(crate::errors::TryReserveError::from_hashbrown)?; + let items = &self.items; + let state = &self.tables.state; self.tables .k1_to_item - .try_reserve(additional) + .try_reserve(additional, |ix| state.hash_one(items[*ix].key1())) .map_err(crate::errors::TryReserveError::from_hashbrown)?; self.tables .k2_to_item - .try_reserve(additional) + .try_reserve(additional, |ix| state.hash_one(items[*ix].key2())) .map_err(crate::errors::TryReserveError::from_hashbrown)?; Ok(()) } @@ -864,8 +872,14 @@ impl BiHashMap { /// ``` pub fn shrink_to_fit(&mut self) { self.items.shrink_to_fit(); - self.tables.k1_to_item.shrink_to_fit(); - self.tables.k2_to_item.shrink_to_fit(); + let items = &self.items; + let state = &self.tables.state; + self.tables + .k1_to_item + .shrink_to_fit(|ix| state.hash_one(items[*ix].key1())); + self.tables + .k2_to_item + .shrink_to_fit(|ix| state.hash_one(items[*ix].key2())); } /// Shrinks the capacity of the map with a lower limit. It will drop @@ -911,8 +925,14 @@ impl BiHashMap { /// ``` pub fn shrink_to(&mut self, min_capacity: usize) { self.items.shrink_to(min_capacity); - self.tables.k1_to_item.shrink_to(min_capacity); - self.tables.k2_to_item.shrink_to(min_capacity); + let items = &self.items; + let state = &self.tables.state; + self.tables + .k1_to_item + .shrink_to(min_capacity, |ix| state.hash_one(items[*ix].key1())); + self.tables + .k2_to_item + .shrink_to(min_capacity, |ix| state.hash_one(items[*ix].key2())); } /// Returns an iterator over all items in the map. diff --git a/crates/iddqd/src/id_hash_map/imp.rs b/crates/iddqd/src/id_hash_map/imp.rs index 2efdcdf..952cc9b 100644 --- a/crates/iddqd/src/id_hash_map/imp.rs +++ b/crates/iddqd/src/id_hash_map/imp.rs @@ -641,7 +641,11 @@ impl IdHashMap { /// ``` pub fn reserve(&mut self, additional: usize) { self.items.reserve(additional); - self.tables.key_to_item.reserve(additional); + let items = &self.items; + let state = &self.tables.state; + self.tables + .key_to_item + .reserve(additional, |ix| state.hash_one(items[*ix].key())); } /// Tries to reserve capacity for at least `additional` more elements to be @@ -693,9 +697,11 @@ impl IdHashMap { self.items .try_reserve(additional) .map_err(crate::errors::TryReserveError::from_hashbrown)?; + let items = &self.items; + let state = &self.tables.state; self.tables .key_to_item - .try_reserve(additional) + .try_reserve(additional, |ix| state.hash_one(items[*ix].key())) .map_err(crate::errors::TryReserveError::from_hashbrown)?; Ok(()) } @@ -734,7 +740,11 @@ impl IdHashMap { /// ``` pub fn shrink_to_fit(&mut self) { self.items.shrink_to_fit(); - self.tables.key_to_item.shrink_to_fit(); + let items = &self.items; + let state = &self.tables.state; + self.tables + .key_to_item + .shrink_to_fit(|ix| state.hash_one(items[*ix].key())); } /// Shrinks the capacity of the map with a lower limit. It will drop @@ -776,7 +786,11 @@ impl IdHashMap { /// ``` pub fn shrink_to(&mut self, min_capacity: usize) { self.items.shrink_to(min_capacity); - self.tables.key_to_item.shrink_to(min_capacity); + let items = &self.items; + let state = &self.tables.state; + self.tables + .key_to_item + .shrink_to(min_capacity, |ix| state.hash_one(items[*ix].key())); } /// Iterates over the items in the map. diff --git a/crates/iddqd/src/support/hash_table.rs b/crates/iddqd/src/support/hash_table.rs index d9c87c6..0341e84 100644 --- a/crates/iddqd/src/support/hash_table.rs +++ b/crates/iddqd/src/support/hash_table.rs @@ -173,29 +173,50 @@ impl MapHashTable { } /// Reserves capacity for at least `additional` more items. + /// + /// `hasher` is invoked for every entry that hashbrown has to re-slot during + /// a growth rehash, and must return the same hash that was used to insert + /// that entry originally. Anything else silently corrupts the table: all + /// surviving entries land in the same bucket, and subsequent lookups — + /// which probe using the real key hash — miss. #[inline] - pub(crate) fn reserve(&mut self, additional: usize) { - self.items.reserve(additional, |_| 0); + pub(crate) fn reserve( + &mut self, + additional: usize, + hasher: impl Fn(&usize) -> u64, + ) { + self.items.reserve(additional, hasher); } /// Shrinks the capacity of the hash table as much as possible. + /// + /// See [`Self::reserve`] for the contract `hasher` must satisfy. #[inline] - pub(crate) fn shrink_to_fit(&mut self) { - self.items.shrink_to_fit(|_| 0); + pub(crate) fn shrink_to_fit(&mut self, hasher: impl Fn(&usize) -> u64) { + self.items.shrink_to_fit(hasher); } /// Shrinks the capacity of the hash table with a lower limit. + /// + /// See [`Self::reserve`] for the contract `hasher` must satisfy. #[inline] - pub(crate) fn shrink_to(&mut self, min_capacity: usize) { - self.items.shrink_to(min_capacity, |_| 0); + pub(crate) fn shrink_to( + &mut self, + min_capacity: usize, + hasher: impl Fn(&usize) -> u64, + ) { + self.items.shrink_to(min_capacity, hasher); } /// Tries to reserve capacity for at least `additional` more items. + /// + /// See [`Self::reserve`] for the contract `hasher` must satisfy. #[inline] pub(crate) fn try_reserve( &mut self, additional: usize, + hasher: impl Fn(&usize) -> u64, ) -> Result<(), hashbrown::TryReserveError> { - self.items.try_reserve(additional, |_| 0) + self.items.try_reserve(additional, hasher) } } diff --git a/crates/iddqd/src/tri_hash_map/imp.rs b/crates/iddqd/src/tri_hash_map/imp.rs index 1e0c25c..3bb4f98 100644 --- a/crates/iddqd/src/tri_hash_map/imp.rs +++ b/crates/iddqd/src/tri_hash_map/imp.rs @@ -854,9 +854,17 @@ impl TriHashMap { /// ``` pub fn reserve(&mut self, additional: usize) { self.items.reserve(additional); - self.tables.k1_to_item.reserve(additional); - self.tables.k2_to_item.reserve(additional); - self.tables.k3_to_item.reserve(additional); + let items = &self.items; + let state = &self.tables.state; + self.tables + .k1_to_item + .reserve(additional, |ix| state.hash_one(items[*ix].key1())); + self.tables + .k2_to_item + .reserve(additional, |ix| state.hash_one(items[*ix].key2())); + self.tables + .k3_to_item + .reserve(additional, |ix| state.hash_one(items[*ix].key3())); } /// Tries to reserve capacity for at least `additional` more elements to be @@ -917,17 +925,19 @@ impl TriHashMap { self.items .try_reserve(additional) .map_err(crate::errors::TryReserveError::from_hashbrown)?; + let items = &self.items; + let state = &self.tables.state; self.tables .k1_to_item - .try_reserve(additional) + .try_reserve(additional, |ix| state.hash_one(items[*ix].key1())) .map_err(crate::errors::TryReserveError::from_hashbrown)?; self.tables .k2_to_item - .try_reserve(additional) + .try_reserve(additional, |ix| state.hash_one(items[*ix].key2())) .map_err(crate::errors::TryReserveError::from_hashbrown)?; self.tables .k3_to_item - .try_reserve(additional) + .try_reserve(additional, |ix| state.hash_one(items[*ix].key3())) .map_err(crate::errors::TryReserveError::from_hashbrown)?; Ok(()) } @@ -985,9 +995,17 @@ impl TriHashMap { /// ``` pub fn shrink_to_fit(&mut self) { self.items.shrink_to_fit(); - self.tables.k1_to_item.shrink_to_fit(); - self.tables.k2_to_item.shrink_to_fit(); - self.tables.k3_to_item.shrink_to_fit(); + let items = &self.items; + let state = &self.tables.state; + self.tables + .k1_to_item + .shrink_to_fit(|ix| state.hash_one(items[*ix].key1())); + self.tables + .k2_to_item + .shrink_to_fit(|ix| state.hash_one(items[*ix].key2())); + self.tables + .k3_to_item + .shrink_to_fit(|ix| state.hash_one(items[*ix].key3())); } /// Shrinks the capacity of the map with a lower limit. It will drop @@ -1048,9 +1066,17 @@ impl TriHashMap { /// ``` pub fn shrink_to(&mut self, min_capacity: usize) { self.items.shrink_to(min_capacity); - self.tables.k1_to_item.shrink_to(min_capacity); - self.tables.k2_to_item.shrink_to(min_capacity); - self.tables.k3_to_item.shrink_to(min_capacity); + let items = &self.items; + let state = &self.tables.state; + self.tables + .k1_to_item + .shrink_to(min_capacity, |ix| state.hash_one(items[*ix].key1())); + self.tables + .k2_to_item + .shrink_to(min_capacity, |ix| state.hash_one(items[*ix].key2())); + self.tables + .k3_to_item + .shrink_to(min_capacity, |ix| state.hash_one(items[*ix].key3())); } /// Iterates over the items in the map. diff --git a/crates/iddqd/tests/integration/bi_hash_map.rs b/crates/iddqd/tests/integration/bi_hash_map.rs index c38ebce..0ec1aa0 100644 --- a/crates/iddqd/tests/integration/bi_hash_map.rs +++ b/crates/iddqd/tests/integration/bi_hash_map.rs @@ -242,14 +242,29 @@ enum Operation { #[weight(2)] RetainModulo(#[strategy(0..3_u8)] u8, #[strategy(1..4_u8)] u8, bool), Clear, + // `additional` is kept modest so that reservations frequently + // exceed the current `growth_left` and so trigger hashbrown's + // rehash path. + Reserve(#[strategy(0..256_usize)] usize), + TryReserve(#[strategy(0..256_usize)] usize), + ShrinkToFit, + ShrinkTo(#[strategy(0..256_usize)] usize), } impl Operation { fn compactness_change(&self) -> CompactnessChange { match self { + // `shrink_to_fit` / `shrink_to` flow through hashbrown's + // rehash path the same way `reserve` does; like `reserve` + // they touch the allocation, not the item set's index + // space, so compactness is unchanged. Operation::InsertUnique(_) | Operation::Get1(_) - | Operation::Get2(_) => CompactnessChange::NoChange, + | Operation::Get2(_) + | Operation::Reserve(_) + | Operation::TryReserve(_) + | Operation::ShrinkToFit + | Operation::ShrinkTo(_) => CompactnessChange::NoChange, // The act of removing items, including calls to insert_overwrite, // can make the map non-compact. Operation::InsertOverwrite(_) @@ -366,6 +381,31 @@ fn proptest_ops( }); map.validate(compactness).expect("map should be valid"); } + Operation::Reserve(additional) => { + map.reserve(additional); + // `reserve` has no observable effect beyond capacity; the + // naive map has no equivalent. `validate` is the real + // check — it iterates items and asks `find_index` for + // each, which catches a hash-table left mis-bucketed by + // a regrowth rehash. + map.validate(compactness).expect("map should be valid"); + } + Operation::TryReserve(additional) => { + // Mirror `Reserve`; we don't assert `Ok` because the + // allocator could (legitimately) refuse a large request, + // and bailing on that would mask the actual regression + // we care about (silent hash-table corruption). + let _ = map.try_reserve(additional); + map.validate(compactness).expect("map should be valid"); + } + Operation::ShrinkToFit => { + map.shrink_to_fit(); + map.validate(compactness).expect("map should be valid"); + } + Operation::ShrinkTo(min_capacity) => { + map.shrink_to(min_capacity); + map.validate(compactness).expect("map should be valid"); + } Operation::Clear => { map.clear(); naive_map.clear(); diff --git a/crates/iddqd/tests/integration/id_hash_map.rs b/crates/iddqd/tests/integration/id_hash_map.rs index f6529b9..85ad1b5 100644 --- a/crates/iddqd/tests/integration/id_hash_map.rs +++ b/crates/iddqd/tests/integration/id_hash_map.rs @@ -195,14 +195,28 @@ enum Operation { #[weight(2)] RetainModulo(#[strategy(0..3_u8)] u8, #[strategy(1..4_u8)] u8, bool), Clear, + // `additional` is kept modest so that reservations frequently + // exceed the current `growth_left` and so trigger hashbrown's + // rehash path. + Reserve(#[strategy(0..256_usize)] usize), + TryReserve(#[strategy(0..256_usize)] usize), + ShrinkToFit, + ShrinkTo(#[strategy(0..256_usize)] usize), } impl Operation { fn compactness_change(&self) -> CompactnessChange { match self { - Operation::InsertUnique(_) | Operation::Get(_) => { - CompactnessChange::NoChange - } + // `shrink_to_fit` / `shrink_to` flow through hashbrown's + // rehash path the same way `reserve` does; like `reserve` + // they touch the allocation, not the item set's index + // space, so compactness is unchanged. + Operation::InsertUnique(_) + | Operation::Get(_) + | Operation::Reserve(_) + | Operation::TryReserve(_) + | Operation::ShrinkToFit + | Operation::ShrinkTo(_) => CompactnessChange::NoChange, // The act of removing items, including calls to insert_overwrite, // can make the map non-compact. Operation::InsertOverwrite(_) @@ -301,6 +315,31 @@ fn proptest_ops( naive_map.clear(); map.validate(compactness).expect("map should be valid"); } + Operation::Reserve(additional) => { + map.reserve(additional); + // `reserve` has no observable effect beyond capacity; the + // naive map has no equivalent. `validate` is the real + // check — it iterates items and asks `find_index` for + // each, which catches a hash-table left mis-bucketed by + // a regrowth rehash. + map.validate(compactness).expect("map should be valid"); + } + Operation::TryReserve(additional) => { + // Mirror `Reserve`; we don't assert `Ok` because the + // allocator could (legitimately) refuse a large request, + // and bailing on that would mask the actual regression + // we care about (silent hash-table corruption). + let _ = map.try_reserve(additional); + map.validate(compactness).expect("map should be valid"); + } + Operation::ShrinkToFit => { + map.shrink_to_fit(); + map.validate(compactness).expect("map should be valid"); + } + Operation::ShrinkTo(min_capacity) => { + map.shrink_to(min_capacity); + map.validate(compactness).expect("map should be valid"); + } } // Check that the iterators work correctly. diff --git a/crates/iddqd/tests/integration/tri_hash_map.rs b/crates/iddqd/tests/integration/tri_hash_map.rs index 0759dcd..4016809 100644 --- a/crates/iddqd/tests/integration/tri_hash_map.rs +++ b/crates/iddqd/tests/integration/tri_hash_map.rs @@ -271,15 +271,30 @@ enum Operation { #[weight(2)] RetainModulo(#[strategy(0..3_u8)] u8, #[strategy(1..4_u8)] u8, bool), Clear, + // `additional` is kept modest so that reservations frequently + // exceed the current `growth_left` and so trigger hashbrown's + // rehash path. + Reserve(#[strategy(0..256_usize)] usize), + TryReserve(#[strategy(0..256_usize)] usize), + ShrinkToFit, + ShrinkTo(#[strategy(0..256_usize)] usize), } impl Operation { fn compactness_change(&self) -> CompactnessChange { match self { + // `shrink_to_fit` / `shrink_to` flow through hashbrown's + // rehash path the same way `reserve` does; like `reserve` + // they touch the allocation, not the item set's index + // space, so compactness is unchanged. Operation::InsertUnique(_) | Operation::Get1(_) | Operation::Get2(_) - | Operation::Get3(_) => CompactnessChange::NoChange, + | Operation::Get3(_) + | Operation::Reserve(_) + | Operation::TryReserve(_) + | Operation::ShrinkToFit + | Operation::ShrinkTo(_) => CompactnessChange::NoChange, // The act of removing items, including calls to insert_overwrite, // can make the map non-compact. Operation::InsertOverwrite(_) @@ -415,6 +430,31 @@ fn proptest_ops( naive_map.clear(); map.validate(compactness).expect("map should be valid"); } + Operation::Reserve(additional) => { + map.reserve(additional); + // `reserve` has no observable effect beyond capacity; the + // naive map has no equivalent. `validate` is the real + // check — it iterates items and asks `find_index` for + // each, which catches a hash-table left mis-bucketed by + // a regrowth rehash. + map.validate(compactness).expect("map should be valid"); + } + Operation::TryReserve(additional) => { + // Mirror `Reserve`; we don't assert `Ok` because the + // allocator could (legitimately) refuse a large request, + // and bailing on that would mask the actual regression + // we care about (silent hash-table corruption). + let _ = map.try_reserve(additional); + map.validate(compactness).expect("map should be valid"); + } + Operation::ShrinkToFit => { + map.shrink_to_fit(); + map.validate(compactness).expect("map should be valid"); + } + Operation::ShrinkTo(min_capacity) => { + map.shrink_to(min_capacity); + map.validate(compactness).expect("map should be valid"); + } } // Check that the iterators work correctly.