diff --git a/CHANGELOG.md b/CHANGELOG.md index f0122c1064..76742558a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,7 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] - Check for device mismatches when beginning render and compute passes. By @ErichDonGubler in [#6497](https://github.com/gfx-rs/wgpu/pull/6497). - Lower `QUERY_SET_MAX_QUERIES` (and enforced limits) from 8192 to 4096 to match WebGPU spec. By @ErichDonGubler in [#6525](https://github.com/gfx-rs/wgpu/pull/6525). - Allow non-filterable float on texture bindings never used with samplers when using a derived bind group layout. By @ErichDonGubler in [#6531](https://github.com/gfx-rs/wgpu/pull/6531/). +- Replace potentially unsound usage of `PreHashedMap` with `FastHashMap`. By @jamienicol in [#6541](https://github.com/gfx-rs/wgpu/pull/6541). #### Naga diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index 2c511baa50..754d1d91f0 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -32,7 +32,7 @@ use crate::{ }, validation::{self, validate_color_attachment_bytes_per_sample}, weak_vec::WeakVec, - FastHashMap, LabelHelpers, PreHashedKey, PreHashedMap, + FastHashMap, LabelHelpers, }; use arrayvec::ArrayVec; @@ -2712,18 +2712,18 @@ impl Device { derived_group_layouts.pop(); } - let mut unique_bind_group_layouts = PreHashedMap::default(); + let mut unique_bind_group_layouts = FastHashMap::default(); let bind_group_layouts = derived_group_layouts .into_iter() .map(|mut bgl_entry_map| { bgl_entry_map.sort(); - match unique_bind_group_layouts.entry(PreHashedKey::from_key(&bgl_entry_map)) { + match unique_bind_group_layouts.entry(bgl_entry_map) { std::collections::hash_map::Entry::Occupied(v) => Ok(Arc::clone(v.get())), std::collections::hash_map::Entry::Vacant(e) => { match self.create_bind_group_layout( &None, - bgl_entry_map, + e.key().clone(), bgl::Origin::Derived, ) { Ok(bgl) => { diff --git a/wgpu-core/src/hash_utils.rs b/wgpu-core/src/hash_utils.rs index f44aad2f1a..056c84f539 100644 --- a/wgpu-core/src/hash_utils.rs +++ b/wgpu-core/src/hash_utils.rs @@ -12,75 +12,3 @@ pub type FastHashSet = /// IndexMap using a fast, non-cryptographic hash algorithm. pub type FastIndexMap = indexmap::IndexMap>; - -/// HashMap that uses pre-hashed keys and an identity hasher. -/// -/// This is useful when you only need the key to lookup the value, and don't need to store the key, -/// particularly when the key is large. -pub type PreHashedMap = - std::collections::HashMap, V, std::hash::BuildHasherDefault>; - -/// A pre-hashed key using FxHash which allows the hashing operation to be disconnected -/// from the storage in the map. -pub struct PreHashedKey(u64, std::marker::PhantomData K>); - -impl std::fmt::Debug for PreHashedKey { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_tuple("PreHashedKey").field(&self.0).finish() - } -} - -impl Copy for PreHashedKey {} - -impl Clone for PreHashedKey { - fn clone(&self) -> Self { - *self - } -} - -impl PartialEq for PreHashedKey { - fn eq(&self, other: &Self) -> bool { - self.0 == other.0 - } -} - -impl Eq for PreHashedKey {} - -impl std::hash::Hash for PreHashedKey { - fn hash(&self, state: &mut H) { - self.0.hash(state); - } -} - -impl PreHashedKey { - pub fn from_key(key: &K) -> Self { - use std::hash::Hasher; - - let mut hasher = rustc_hash::FxHasher::default(); - key.hash(&mut hasher); - Self(hasher.finish(), std::marker::PhantomData) - } -} - -/// A hasher which does nothing. Useful for when you want to use a map with pre-hashed keys. -/// -/// When hashing with this hasher, you must provide exactly 8 bytes. Multiple calls to `write` -/// will overwrite the previous value. -#[derive(Default)] -pub struct IdentityHasher { - hash: u64, -} - -impl std::hash::Hasher for IdentityHasher { - fn write(&mut self, bytes: &[u8]) { - self.hash = u64::from_ne_bytes( - bytes - .try_into() - .expect("identity hasher must be given exactly 8 bytes"), - ); - } - - fn finish(&self) -> u64 { - self.hash - } -} diff --git a/wgpu-core/src/pool.rs b/wgpu-core/src/pool.rs index 7d17f3a7a3..d14b8162e3 100644 --- a/wgpu-core/src/pool.rs +++ b/wgpu-core/src/pool.rs @@ -7,16 +7,13 @@ use std::{ use once_cell::sync::OnceCell; use crate::lock::{rank, Mutex}; -use crate::{PreHashedKey, PreHashedMap}; +use crate::FastHashMap; type SlotInner = Weak; type ResourcePoolSlot = Arc>>; pub struct ResourcePool { - // We use a pre-hashed map as we never actually need to read the keys. - // - // This additionally allows us to not need to hash more than once on get_or_init. - inner: Mutex>>, + inner: Mutex>>, } impl ResourcePool { @@ -35,9 +32,6 @@ impl ResourcePool { where F: FnOnce(K) -> Result, E>, { - // Hash the key outside of the lock. - let hashed_key = PreHashedKey::from_key(&key); - // We can't prove at compile time that these will only ever be consumed once, // so we need to do the check at runtime. let mut key = Some(key); @@ -46,7 +40,7 @@ impl ResourcePool { 'race: loop { let mut map_guard = self.inner.lock(); - let entry = match map_guard.entry(hashed_key) { + let entry = match map_guard.entry(key.clone().unwrap()) { // An entry exists for this resource. // // We know that either: @@ -86,9 +80,11 @@ impl ResourcePool { return Ok(strong); } - // The resource is in the process of being dropped, because upgrade failed. The entry still exists in the map, but it points to nothing. + // The resource is in the process of being dropped, because upgrade failed. + // The entry still exists in the map, but it points to nothing. // - // We're in a race with the drop implementation of the resource, so lets just go around again. When we go around again: + // We're in a race with the drop implementation of the resource, + // so lets just go around again. When we go around again: // - If the entry exists, we might need to go around a few more times. // - If the entry doesn't exist, we'll create a new one. continue 'race; @@ -101,13 +97,11 @@ impl ResourcePool { /// /// [`BindGroupLayout`]: crate::binding_model::BindGroupLayout pub fn remove(&self, key: &K) { - let hashed_key = PreHashedKey::from_key(key); - let mut map_guard = self.inner.lock(); // Weak::upgrade will be failing long before this code is called. All threads trying to access the resource will be spinning, // waiting for the entry to be removed. It is safe to remove the entry from the map. - map_guard.remove(&hashed_key); + map_guard.remove(key); } }