Skip to content

Commit

Permalink
Proper prehashing (#3963)
Browse files Browse the repository at this point in the history
For some keys, it is too expensive to hash them on every lookup. Historically in Bevy, we have regrettably done the "wrong" thing in these cases (pre-computing hashes, then re-hashing them) because Rust's built in hashed collections don't give us the tools we need to do otherwise. Doing this is "wrong" because two different values can result in the same hash. Hashed collections generally get around this by falling back to equality checks on hash collisions. You can't do that if the key _is_ the hash. Additionally, re-hashing a hash increase the odds of collision!
 
#3959 needs pre-hashing to be viable, so I decided to finally properly solve the problem. The solution involves two different changes:

1. A new generalized "pre-hashing" solution in bevy_utils: `Hashed<T>` types, which store a value alongside a pre-computed hash. And `PreHashMap<K, V>` (which uses `Hashed<T>` internally) . `PreHashMap` is just an alias for a normal HashMap that uses `Hashed<T>` as the key and a new `PassHash` implementation as the Hasher. 
2. Replacing the `std::collections` re-exports in `bevy_utils` with equivalent `hashbrown` impls. Avoiding re-hashes requires the `raw_entry_mut` api, which isn't stabilized yet (and may never be ... `entry_ref` has favor now, but also isn't available yet). If std's HashMap ever provides the tools we need, we can move back to that. The latest version of `hashbrown` adds support for the `entity_ref` api, so we can move to that in preparation for an std migration, if thats the direction they seem to be going in. Note that adding hashbrown doesn't increase our dependency count because it was already in our tree.

In addition to providing these core tools, I also ported the "table identity hashing" in `bevy_ecs` to `raw_entry_mut`, which was a particularly egregious case.

The biggest outstanding case is `AssetPathId`, which stores a pre-hash. We need AssetPathId to be cheaply clone-able (and ideally Copy), but `Hashed<AssetPath>` requires ownership of the AssetPath, which makes cloning ids way more expensive. We could consider doing `Hashed<Arc<AssetPath>>`, but cloning an arc is still a non-trivial expensive that needs to be considered. I would like to handle this in a separate PR. And given that we will be re-evaluating the Bevy Assets implementation in the very near future, I'd prefer to hold off until after that conversation is concluded.
  • Loading branch information
cart committed Feb 18, 2022
1 parent c4f132a commit b3a1db6
Show file tree
Hide file tree
Showing 12 changed files with 186 additions and 181 deletions.
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/asset_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ use anyhow::Result;
use bevy_ecs::system::{Res, ResMut};
use bevy_log::warn;
use bevy_tasks::TaskPool;
use bevy_utils::{HashMap, Uuid};
use bevy_utils::{Entry, HashMap, Uuid};
use crossbeam_channel::TryRecvError;
use parking_lot::{Mutex, RwLock};
use std::{collections::hash_map::Entry, path::Path, sync::Arc};
use std::{path::Path, sync::Arc};
use thiserror::Error;

/// Errors that occur while loading assets with an `AssetServer`
Expand Down
3 changes: 1 addition & 2 deletions crates/bevy_ecs/src/entity/map_entities.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::entity::Entity;
use bevy_utils::HashMap;
use std::collections::hash_map::Entry;
use bevy_utils::{Entry, HashMap};
use thiserror::Error;

#[derive(Error, Debug)]
Expand Down
30 changes: 16 additions & 14 deletions crates/bevy_ecs/src/storage/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ use crate::{
entity::Entity,
storage::{BlobVec, SparseSet},
};
use bevy_utils::{AHasher, HashMap};
use bevy_utils::HashMap;
use std::{
cell::UnsafeCell,
hash::{Hash, Hasher},
ops::{Index, IndexMut},
ptr::NonNull,
};
Expand Down Expand Up @@ -415,7 +414,7 @@ impl Table {
/// Can be accessed via [`Storages`](crate::storage::Storages)
pub struct Tables {
tables: Vec<Table>,
table_ids: HashMap<u64, TableId>,
table_ids: HashMap<Vec<ComponentId>, TableId>,
}

impl Default for Tables {
Expand Down Expand Up @@ -472,18 +471,21 @@ impl Tables {
component_ids: &[ComponentId],
components: &Components,
) -> TableId {
let mut hasher = AHasher::default();
component_ids.hash(&mut hasher);
let hash = hasher.finish();
let tables = &mut self.tables;
*self.table_ids.entry(hash).or_insert_with(move || {
let mut table = Table::with_capacity(0, component_ids.len());
for component_id in component_ids.iter() {
table.add_column(components.get_info_unchecked(*component_id));
}
tables.push(table);
TableId(tables.len() - 1)
})
let (_key, value) = self
.table_ids
.raw_entry_mut()
.from_key(component_ids)
.or_insert_with(|| {
let mut table = Table::with_capacity(0, component_ids.len());
for component_id in component_ids.iter() {
table.add_column(components.get_info_unchecked(*component_id));
}
tables.push(table);
(component_ids.to_vec(), TableId(tables.len() - 1))
});

*value
}

pub fn iter(&self) -> std::slice::Iter<'_, Table> {
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_input/src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ use bevy_ecs::schedule::State;
/// * Call the [`Input::release`] method for each release event.
/// * Call the [`Input::clear`] method at each frame start, before processing events.
#[derive(Debug, Clone)]
pub struct Input<T> {
pub struct Input<T: Eq + Hash> {
pressed: HashSet<T>,
just_pressed: HashSet<T>,
just_released: HashSet<T>,
}

impl<T> Default for Input<T> {
impl<T: Eq + Hash> Default for Input<T> {
fn default() -> Self {
Self {
pressed: Default::default(),
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_reflect/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ thiserror = "1.0"
serde = "1"
smallvec = { version = "1.6", features = ["serde", "union", "const_generics"], optional = true }
glam = { version = "0.20.0", features = ["serde"], optional = true }
hashbrown = { version = "0.11", features = ["serde"], optional = true }

[dev-dependencies]
ron = "0.7.0"
2 changes: 1 addition & 1 deletion crates/bevy_reflect/src/impls/std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
};

use bevy_reflect_derive::{impl_from_reflect_value, impl_reflect_value};
use bevy_utils::{AHashExt, Duration, HashMap, HashSet};
use bevy_utils::{Duration, HashMap, HashSet};
use serde::{Deserialize, Serialize};
use std::{
any::Any,
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_reflect/src/map.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{any::Any, collections::hash_map::Entry};
use std::any::Any;

use bevy_utils::HashMap;
use bevy_utils::{Entry, HashMap};

use crate::{serde::Serializable, Reflect, ReflectMut, ReflectRef};

Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_reflect/src/struct_trait.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{serde::Serializable, Reflect, ReflectMut, ReflectRef};
use bevy_utils::HashMap;
use std::{any::Any, borrow::Cow, collections::hash_map::Entry};
use bevy_utils::{Entry, HashMap};
use std::{any::Any, borrow::Cow};

/// A reflected Rust regular struct type.
///
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_render/src/render_resource/pipeline_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use crate::{
use bevy_app::EventReader;
use bevy_asset::{AssetEvent, Assets, Handle};
use bevy_ecs::system::{Res, ResMut};
use bevy_utils::{tracing::error, HashMap, HashSet};
use std::{collections::hash_map::Entry, hash::Hash, ops::Deref, sync::Arc};
use bevy_utils::{tracing::error, Entry, HashMap, HashSet};
use std::{hash::Hash, ops::Deref, sync::Arc};
use thiserror::Error;
use wgpu::{PipelineLayoutDescriptor, ShaderModule, VertexBufferLayout};

Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_render/src/texture/texture_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
renderer::RenderDevice,
};
use bevy_ecs::prelude::ResMut;
use bevy_utils::HashMap;
use bevy_utils::{Entry, HashMap};
use wgpu::{TextureDescriptor, TextureViewDescriptor};

/// The internal representation of a [`CachedTexture`] used to track whether it was recently used
Expand Down Expand Up @@ -39,7 +39,7 @@ impl TextureCache {
descriptor: TextureDescriptor<'static>,
) -> CachedTexture {
match self.textures.entry(descriptor) {
std::collections::hash_map::Entry::Occupied(mut entry) => {
Entry::Occupied(mut entry) => {
for texture in entry.get_mut().iter_mut() {
if !texture.taken {
texture.frames_since_last_use = 0;
Expand All @@ -64,7 +64,7 @@ impl TextureCache {
default_view,
}
}
std::collections::hash_map::Entry::Vacant(entry) => {
Entry::Vacant(entry) => {
let texture = render_device.create_texture(entry.key());
let default_view = texture.create_view(&TextureViewDescriptor::default());
entry.insert(vec![CachedTextureMeta {
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ ahash = "0.7.0"
tracing = {version = "0.1", features = ["release_max_level_info"]}
instant = { version = "0.1", features = ["wasm-bindgen"] }
uuid = { version = "0.8", features = ["v4", "serde"] }
hashbrown = { version = "0.11", features = ["serde"] }

[target.'cfg(target_arch = "wasm32")'.dependencies]
getrandom = {version = "0.2.0", features = ["js"]}
Expand Down
Loading

0 comments on commit b3a1db6

Please sign in to comment.