Skip to content

while removing from hash maps, only do hash lookups once #254

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

Merged
sunshowers merged 1 commit into from
May 22, 2026
Merged

while removing from hash maps, only do hash lookups once #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions crates/iddqd/src/bi_hash_map/imp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2189,32 +2189,32 @@ impl<T: BiHashItem, S: Clone + BuildHasher, A: Allocator> BiHashMap<T, S, A> {
&mut self,
remove_index: ItemIndex,
) -> Option<T> {
// For panic safety, look up both table entries while `self.items` still
// holds the value, then remove from both tables and items in sequence.
// hashbrown's `find_entry` is panic-safe under user-`Hash`/`Eq` panics
// (the table is not mutated until `OccupiedEntry::remove` is called),
// so a panic during the lookups leaves items and both tables
// unmodified.
// For panic safety, compute both key hashes and look up both table
// entries while `self.items` still holds the value, then remove from
// both tables and items in sequence. These lookups deliberately match
// by `ItemIndex` rather than by user `Eq`: at this point we already
// know which item is being removed, and user `Eq` might be
// pathological. hashbrown's `find_entry_by_hash` is panic-safe because
// the table is not mutated until `OccupiedEntry::remove` is called, so
// a panic while hashing leaves items and both tables unmodified.
let item = self.items.get(remove_index)?;
let key1 = item.key1();
let key2 = item.key2();
let state = &self.tables.state;
let Ok(entry1) =
self.tables
.k1_to_item
.find_entry(state, &key1, |index| self.items[index].key1())
let hash1 = state.hash_one(item.key1());
let hash2 = state.hash_one(item.key2());
let Ok(entry1) = self
.tables
.k1_to_item
.find_entry_by_hash(hash1, |index| index == remove_index)
else {
panic!("remove_index {remove_index} not found in k1_to_item");
};
let Ok(entry2) =
self.tables
.k2_to_item
.find_entry(state, &key2, |index| self.items[index].key2())
let Ok(entry2) = self
.tables
.k2_to_item
.find_entry_by_hash(hash2, |index| index == remove_index)
else {
panic!("remove_index {remove_index} not found in k2_to_item");
};
// Drop the keys so that `self.items` can be mutated below.
drop((key1, key2));
entry1.remove();
entry2.remove();
Some(
Expand Down
31 changes: 16 additions & 15 deletions crates/iddqd/src/id_hash_map/imp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,24 +1426,25 @@ impl<T: IdHashItem, S: Clone + BuildHasher, A: Allocator> IdHashMap<T, S, A> {
&mut self,
remove_index: ItemIndex,
) -> Option<T> {
// For panic safety, look up the table entry while `self.items` still
// holds the value, then remove from the table and items in sequence.
// hashbrown's `find_entry` is panic-safe under user-`Hash`/`Eq` panics
// (the table is not mutated until `OccupiedEntry::remove` is called),
// so a panic during the lookup leaves both items and the table
// unmodified.
let key = self.items.get(remove_index)?.key();
// For panic safety, compute the key hash and look up the table entry
// while `self.items` still holds the value, then remove from the table
// and items in sequence. This lookup deliberately matches by
// `ItemIndex` rather than by user `Eq`: at this point we already know
// which item is being removed, and user `Eq` might be pathological.
// hashbrown's `find_entry_by_hash` is panic-safe because the table is
// not mutated until `OccupiedEntry::remove` is called, so a panic while
// hashing leaves both items and the table unmodified.
let item = self.items.get(remove_index)?;
let state = &self.tables.state;
let Ok(item) =
self.tables
.key_to_item
.find_entry(state, &key, |index| self.items[index].key())
let hash = state.hash_one(item.key());
let Ok(entry) = self
.tables
.key_to_item
.find_entry_by_hash(hash, |index| index == remove_index)
else {
panic!("we just looked this item up");
panic!("remove_index {remove_index} not found in key_to_item");
};
// Drop the key so that `self.items` can be mutated below.
drop(key);
item.remove();
entry.remove();
Some(
self.items
.remove(remove_index)
Expand Down
19 changes: 0 additions & 19 deletions crates/iddqd/src/support/hash_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use super::{
use crate::internal::{TableValidationError, ValidateCompact};
use alloc::{collections::BTreeSet, vec::Vec};
use core::{
borrow::Borrow,
fmt,
hash::{BuildHasher, Hash},
};
Expand Down Expand Up @@ -129,24 +128,6 @@ impl<A: Allocator> MapHashTable<A> {
)
}

pub(crate) fn find_entry<S: BuildHasher, K, Q, F>(
&mut self,
state: &S,
key: &Q,
lookup: F,
) -> Result<
OccupiedEntry<'_, ItemIndex, AllocWrapper<A>>,
AbsentEntry<'_, ItemIndex, AllocWrapper<A>>,
>
where
F: Fn(ItemIndex) -> K,
K: Hash + Eq + Borrow<Q>,
Q: ?Sized + Hash + Eq,
{
let hash = state.hash_one(key);
self.items.find_entry(hash, |index| lookup(*index).borrow() == key)
}

pub(crate) fn find_entry_by_hash<F>(
&mut self,
hash: u64,
Expand Down
47 changes: 24 additions & 23 deletions crates/iddqd/src/tri_hash_map/imp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2730,40 +2730,41 @@ impl<T: TriHashItem, S: Clone + BuildHasher, A: Allocator> TriHashMap<T, S, A> {
&mut self,
remove_index: ItemIndex,
) -> Option<T> {
// For panic safety, look up all three table entries while `self.items`
// still holds the value, then remove from all three tables and items
// in sequence. hashbrown's `find_entry` is panic-safe under
// user-`Hash`/`Eq` panics (the table is not mutated until
// `OccupiedEntry::remove` is called), so a panic during the lookups
// leaves items and all three tables unmodified.
// For panic safety, compute all three key hashes and look up all three
// table entries while `self.items` still holds the value, then remove
// from all three tables and items in sequence. These lookups
// deliberately match by `ItemIndex` rather than by user `Eq`: at this
// point we already know which item is being removed, and user `Eq`
// might be pathological. hashbrown's `find_entry_by_hash` is
// panic-safe because the table is not mutated until
// `OccupiedEntry::remove` is called, so a panic while hashing leaves
// items and all three tables unmodified.
let item = self.items.get(remove_index)?;
let key1 = item.key1();
let key2 = item.key2();
let key3 = item.key3();
let state = &self.tables.state;
let Ok(entry1) =
self.tables
.k1_to_item
.find_entry(state, &key1, |index| self.items[index].key1())
let hash1 = state.hash_one(item.key1());
let hash2 = state.hash_one(item.key2());
let hash3 = state.hash_one(item.key3());
let Ok(entry1) = self
.tables
.k1_to_item
.find_entry_by_hash(hash1, |index| index == remove_index)
else {
panic!("remove_index {remove_index} not found in k1_to_item");
};
let Ok(entry2) =
self.tables
.k2_to_item
.find_entry(state, &key2, |index| self.items[index].key2())
let Ok(entry2) = self
.tables
.k2_to_item
.find_entry_by_hash(hash2, |index| index == remove_index)
else {
panic!("remove_index {remove_index} not found in k2_to_item");
};
let Ok(entry3) =
self.tables
.k3_to_item
.find_entry(state, &key3, |index| self.items[index].key3())
let Ok(entry3) = self
.tables
.k3_to_item
.find_entry_by_hash(hash3, |index| index == remove_index)
else {
panic!("remove_index {remove_index} not found in k3_to_item");
};
// Drop the keys so that `self.items` can be mutated below.
drop((key1, key2, key3));
entry1.remove();
entry2.remove();
entry3.remove();
Expand Down
Loading
Loading