Skip to content

Commit

Permalink
Use immutable key for HashMap and HashSet (bevyengine#12086)
Browse files Browse the repository at this point in the history
# Objective

Memory usage optimisation

## Solution

`HashMap` and `HashSet`'s keys are immutable. So using mutable types
like `String`, `Vec<T>`, or `PathBuf` as a key is a waste of memory:
they have an extra `usize` for their capacity and may have spare
capacity.
This PR replaces these types by their immutable equivalents `Box<str>`,
`Box<[T]>`, and `Box<Path>`.

For more context, I recommend watching the [Use Arc Instead of
Vec](https://www.youtube.com/watch?v=A4cKi7PTJSs) video.

---------

Co-authored-by: James Liu <contact@jamessliu.com>
  • Loading branch information
2 people authored and msvbg committed Feb 26, 2024
1 parent dfb1b39 commit 09599f1
Show file tree
Hide file tree
Showing 17 changed files with 72 additions and 79 deletions.
4 changes: 2 additions & 2 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ pub struct App {
pub main_schedule_label: InternedScheduleLabel,
sub_apps: HashMap<InternedAppLabel, SubApp>,
plugin_registry: Vec<Box<dyn Plugin>>,
plugin_name_added: HashSet<String>,
plugin_name_added: HashSet<Box<str>>,
/// A private counter to prevent incorrect calls to `App::run()` from `Plugin::build()`
building_plugin_depth: usize,
plugins_state: PluginsState,
Expand Down Expand Up @@ -642,7 +642,7 @@ impl App {
plugin: Box<dyn Plugin>,
) -> Result<&mut Self, AppError> {
debug!("added plugin: {}", plugin.name());
if plugin.is_unique() && !self.plugin_name_added.insert(plugin.name().to_string()) {
if plugin.is_unique() && !self.plugin_name_added.insert(plugin.name().into()) {
Err(AppError::DuplicatePlugin {
plugin_name: plugin.name().to_string(),
})?;
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_asset/src/io/embedded/embedded_watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub struct EmbeddedWatcher {
impl EmbeddedWatcher {
pub fn new(
dir: Dir,
root_paths: Arc<RwLock<HashMap<PathBuf, PathBuf>>>,
root_paths: Arc<RwLock<HashMap<Box<Path>, PathBuf>>>,
sender: crossbeam_channel::Sender<AssetSourceEvent>,
debounce_wait_time: Duration,
) -> Self {
Expand All @@ -49,7 +49,7 @@ impl AssetWatcher for EmbeddedWatcher {}
/// the initial static bytes from the file embedded in the binary.
pub(crate) struct EmbeddedEventHandler {
sender: crossbeam_channel::Sender<AssetSourceEvent>,
root_paths: Arc<RwLock<HashMap<PathBuf, PathBuf>>>,
root_paths: Arc<RwLock<HashMap<Box<Path>, PathBuf>>>,
root: PathBuf,
dir: Dir,
last_event: Option<AssetSourceEvent>,
Expand All @@ -61,7 +61,7 @@ impl FilesystemEventHandler for EmbeddedEventHandler {

fn get_path(&self, absolute_path: &Path) -> Option<(PathBuf, bool)> {
let (local_path, is_meta) = get_asset_path(&self.root, absolute_path);
let final_path = self.root_paths.read().get(&local_path)?.clone();
let final_path = self.root_paths.read().get(local_path.as_path())?.clone();
if is_meta {
warn!("Meta file asset hot-reloading is not supported yet: {final_path:?}");
}
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_asset/src/io/embedded/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub const EMBEDDED: &str = "embedded";
pub struct EmbeddedAssetRegistry {
dir: Dir,
#[cfg(feature = "embedded_watcher")]
root_paths: std::sync::Arc<parking_lot::RwLock<bevy_utils::HashMap<PathBuf, PathBuf>>>,
root_paths: std::sync::Arc<parking_lot::RwLock<bevy_utils::HashMap<Box<Path>, PathBuf>>>,
}

impl EmbeddedAssetRegistry {
Expand All @@ -35,7 +35,7 @@ impl EmbeddedAssetRegistry {
#[cfg(feature = "embedded_watcher")]
self.root_paths
.write()
.insert(full_path.to_owned(), asset_path.to_owned());
.insert(full_path.into(), asset_path.to_owned());
self.dir.insert_asset(asset_path, value);
}

Expand All @@ -48,7 +48,7 @@ impl EmbeddedAssetRegistry {
#[cfg(feature = "embedded_watcher")]
self.root_paths
.write()
.insert(full_path.to_owned(), asset_path.to_owned());
.insert(full_path.into(), asset_path.to_owned());
self.dir.insert_meta(asset_path, value);
}

Expand Down
13 changes: 5 additions & 8 deletions crates/bevy_asset/src/io/gated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,15 @@ use crate::io::{AssetReader, AssetReaderError, PathStream, Reader};
use bevy_utils::{BoxedFuture, HashMap};
use crossbeam_channel::{Receiver, Sender};
use parking_lot::RwLock;
use std::{
path::{Path, PathBuf},
sync::Arc,
};
use std::{path::Path, sync::Arc};

/// A "gated" reader that will prevent asset reads from returning until
/// a given path has been "opened" using [`GateOpener`].
///
/// This is built primarily for unit tests.
pub struct GatedReader<R: AssetReader> {
reader: R,
gates: Arc<RwLock<HashMap<PathBuf, (Sender<()>, Receiver<()>)>>>,
gates: Arc<RwLock<HashMap<Box<Path>, (Sender<()>, Receiver<()>)>>>,
}

impl<R: AssetReader + Clone> Clone for GatedReader<R> {
Expand All @@ -27,7 +24,7 @@ impl<R: AssetReader + Clone> Clone for GatedReader<R> {

/// Opens path "gates" for a [`GatedReader`].
pub struct GateOpener {
gates: Arc<RwLock<HashMap<PathBuf, (Sender<()>, Receiver<()>)>>>,
gates: Arc<RwLock<HashMap<Box<Path>, (Sender<()>, Receiver<()>)>>>,
}

impl GateOpener {
Expand All @@ -36,7 +33,7 @@ impl GateOpener {
pub fn open<P: AsRef<Path>>(&self, path: P) {
let mut gates = self.gates.write();
let gates = gates
.entry(path.as_ref().to_path_buf())
.entry_ref(path.as_ref())
.or_insert_with(crossbeam_channel::unbounded);
gates.0.send(()).unwrap();
}
Expand Down Expand Up @@ -65,7 +62,7 @@ impl<R: AssetReader> AssetReader for GatedReader<R> {
let receiver = {
let mut gates = self.gates.write();
let gates = gates
.entry(path.to_path_buf())
.entry_ref(path.as_ref())
.or_insert_with(crossbeam_channel::unbounded);
gates.1.clone()
};
Expand Down
19 changes: 12 additions & 7 deletions crates/bevy_asset/src/io/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ use std::{

#[derive(Default, Debug)]
struct DirInternal {
assets: HashMap<String, Data>,
metadata: HashMap<String, Data>,
dirs: HashMap<String, Dir>,
assets: HashMap<Box<str>, Data>,
metadata: HashMap<Box<str>, Data>,
dirs: HashMap<Box<str>, Dir>,
path: PathBuf,
}

Expand Down Expand Up @@ -46,7 +46,7 @@ impl Dir {
dir = self.get_or_insert_dir(parent);
}
dir.0.write().assets.insert(
path.file_name().unwrap().to_string_lossy().to_string(),
path.file_name().unwrap().to_string_lossy().into(),
Data {
value: value.into(),
path: path.to_owned(),
Expand All @@ -60,7 +60,7 @@ impl Dir {
dir = self.get_or_insert_dir(parent);
}
dir.0.write().metadata.insert(
path.file_name().unwrap().to_string_lossy().to_string(),
path.file_name().unwrap().to_string_lossy().into(),
Data {
value: value.into(),
path: path.to_owned(),
Expand All @@ -73,7 +73,7 @@ impl Dir {
let mut full_path = PathBuf::new();
for c in path.components() {
full_path.push(c);
let name = c.as_os_str().to_string_lossy().to_string();
let name = c.as_os_str().to_string_lossy().into();
dir = {
let dirs = &mut dir.0.write().dirs;
dirs.entry(name)
Expand Down Expand Up @@ -147,7 +147,12 @@ impl Stream for DirStream {
let dir = this.dir.0.read();

let dir_index = this.dir_index;
if let Some(dir_path) = dir.dirs.keys().nth(dir_index).map(|d| dir.path.join(d)) {
if let Some(dir_path) = dir
.dirs
.keys()
.nth(dir_index)
.map(|d| dir.path.join(d.as_ref()))
{
this.dir_index += 1;
Poll::Ready(Some(dir_path))
} else {
Expand Down
12 changes: 4 additions & 8 deletions crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,10 +451,7 @@ mod tests {
use bevy_utils::{BoxedFuture, Duration, HashMap};
use futures_lite::AsyncReadExt;
use serde::{Deserialize, Serialize};
use std::{
path::{Path, PathBuf},
sync::Arc,
};
use std::{path::Path, sync::Arc};
use thiserror::Error;

#[derive(Asset, TypePath, Debug, Default)]
Expand Down Expand Up @@ -545,7 +542,7 @@ mod tests {
/// A dummy [`CoolText`] asset reader that only succeeds after `failure_count` times it's read from for each asset.
#[derive(Default, Clone)]
pub struct UnstableMemoryAssetReader {
pub attempt_counters: Arc<std::sync::Mutex<HashMap<PathBuf, usize>>>,
pub attempt_counters: Arc<std::sync::Mutex<HashMap<Box<Path>, usize>>>,
pub load_delay: Duration,
memory_reader: MemoryAssetReader,
failure_count: usize,
Expand Down Expand Up @@ -589,13 +586,12 @@ mod tests {
Result<Box<bevy_asset::io::Reader<'a>>, bevy_asset::io::AssetReaderError>,
> {
let attempt_number = {
let key = PathBuf::from(path);
let mut attempt_counters = self.attempt_counters.lock().unwrap();
if let Some(existing) = attempt_counters.get_mut(&key) {
if let Some(existing) = attempt_counters.get_mut(path) {
*existing += 1;
*existing
} else {
attempt_counters.insert(key, 1);
attempt_counters.insert(path.into(), 1);
1
}
};
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/processor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub struct AssetProcessorData {
log: async_lock::RwLock<Option<ProcessorTransactionLog>>,
processors: RwLock<HashMap<&'static str, Arc<dyn ErasedProcessor>>>,
/// Default processors for file extensions
default_processors: RwLock<HashMap<String, &'static str>>,
default_processors: RwLock<HashMap<Box<str>, &'static str>>,
state: async_lock::RwLock<ProcessorState>,
sources: AssetSources,
initialized_sender: async_broadcast::Sender<()>,
Expand Down Expand Up @@ -482,7 +482,7 @@ impl AssetProcessor {
/// Set the default processor for the given `extension`. Make sure `P` is registered with [`AssetProcessor::register_processor`].
pub fn set_default_processor<P: Process>(&self, extension: &str) {
let mut default_processors = self.data.default_processors.write();
default_processors.insert(extension.to_string(), std::any::type_name::<P>());
default_processors.insert(extension.into(), std::any::type_name::<P>());
}

/// Returns the default processor for the given `extension`, if it exists.
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_asset/src/server/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub(crate) struct AssetInfos {
pub(crate) loader_dependants: HashMap<AssetPath<'static>, HashSet<AssetPath<'static>>>,
/// Tracks living labeled assets for a given source asset.
/// This should only be set when watching for changes to avoid unnecessary work.
pub(crate) living_labeled_assets: HashMap<AssetPath<'static>, HashSet<String>>,
pub(crate) living_labeled_assets: HashMap<AssetPath<'static>, HashSet<Box<str>>>,
pub(crate) handle_providers: TypeIdMap<AssetHandleProvider>,
pub(crate) dependency_loaded_event_sender: TypeIdMap<fn(&mut World, UntypedAssetId)>,
pub(crate) dependency_failed_event_sender:
Expand Down Expand Up @@ -113,7 +113,7 @@ impl AssetInfos {
fn create_handle_internal(
infos: &mut HashMap<UntypedAssetId, AssetInfo>,
handle_providers: &TypeIdMap<AssetHandleProvider>,
living_labeled_assets: &mut HashMap<AssetPath<'static>, HashSet<String>>,
living_labeled_assets: &mut HashMap<AssetPath<'static>, HashSet<Box<str>>>,
watching_for_changes: bool,
type_id: TypeId,
path: Option<AssetPath<'static>>,
Expand All @@ -129,7 +129,7 @@ impl AssetInfos {
let mut without_label = path.to_owned();
if let Some(label) = without_label.take_label() {
let labels = living_labeled_assets.entry(without_label).or_default();
labels.insert(label.to_string());
labels.insert(label.as_ref().into());
}
}
}
Expand Down Expand Up @@ -613,7 +613,7 @@ impl AssetInfos {
info: &AssetInfo,
loader_dependants: &mut HashMap<AssetPath<'static>, HashSet<AssetPath<'static>>>,
path: &AssetPath<'static>,
living_labeled_assets: &mut HashMap<AssetPath<'static>, HashSet<String>>,
living_labeled_assets: &mut HashMap<AssetPath<'static>, HashSet<Box<str>>>,
) {
for loader_dependency in info.loader_dependencies.keys() {
if let Some(dependants) = loader_dependants.get_mut(loader_dependency) {
Expand Down Expand Up @@ -642,7 +642,7 @@ impl AssetInfos {
infos: &mut HashMap<UntypedAssetId, AssetInfo>,
path_to_id: &mut HashMap<AssetPath<'static>, TypeIdMap<UntypedAssetId>>,
loader_dependants: &mut HashMap<AssetPath<'static>, HashSet<AssetPath<'static>>>,
living_labeled_assets: &mut HashMap<AssetPath<'static>, HashSet<String>>,
living_labeled_assets: &mut HashMap<AssetPath<'static>, HashSet<Box<str>>>,
watching_for_changes: bool,
id: UntypedAssetId,
) -> bool {
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_asset/src/server/loaders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use thiserror::Error;
pub(crate) struct AssetLoaders {
loaders: Vec<MaybeAssetLoader>,
type_id_to_loaders: TypeIdMap<Vec<usize>>,
extension_to_loaders: HashMap<String, Vec<usize>>,
extension_to_loaders: HashMap<Box<str>, Vec<usize>>,
type_name_to_loader: HashMap<&'static str, usize>,
preregistered_loaders: HashMap<&'static str, usize>,
}
Expand Down Expand Up @@ -44,7 +44,7 @@ impl AssetLoaders {
for extension in loader.extensions() {
let list = self
.extension_to_loaders
.entry(extension.to_string())
.entry((*extension).into())
.or_default();

if !list.is_empty() {
Expand Down Expand Up @@ -105,7 +105,7 @@ impl AssetLoaders {
for extension in extensions {
let list = self
.extension_to_loaders
.entry(extension.to_string())
.entry((*extension).into())
.or_default();

if !list.is_empty() {
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ pub struct Bundles {
/// Cache static [`BundleId`]
bundle_ids: TypeIdMap<BundleId>,
/// Cache dynamic [`BundleId`] with multiple components
dynamic_bundle_ids: HashMap<Vec<ComponentId>, (BundleId, Vec<StorageType>)>,
dynamic_bundle_ids: HashMap<Box<[ComponentId]>, (BundleId, Vec<StorageType>)>,
/// Cache optimized dynamic [`BundleId`] with single component
dynamic_component_bundle_ids: HashMap<ComponentId, (BundleId, StorageType)>,
}
Expand Down Expand Up @@ -871,7 +871,7 @@ impl Bundles {
.from_key(component_ids)
.or_insert_with(|| {
(
Vec::from(component_ids),
component_ids.into(),
initialize_dynamic_bundle(bundle_infos, components, Vec::from(component_ids)),
)
});
Expand Down
7 changes: 2 additions & 5 deletions crates/bevy_ecs/src/storage/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,7 @@ impl Table {
/// Can be accessed via [`Storages`](crate::storage::Storages)
pub struct Tables {
tables: Vec<Table>,
table_ids: HashMap<Vec<ComponentId>, TableId>,
table_ids: HashMap<Box<[ComponentId]>, TableId>,
}

impl Default for Tables {
Expand Down Expand Up @@ -872,10 +872,7 @@ impl Tables {
table = table.add_column(components.get_info_unchecked(*component_id));
}
tables.push(table.build());
(
component_ids.to_vec(),
TableId::from_usize(tables.len() - 1),
)
(component_ids.into(), TableId::from_usize(tables.len() - 1))
});

*value
Expand Down
15 changes: 7 additions & 8 deletions crates/bevy_gltf/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use bevy_scene::Scene;
/// Adds support for glTF file loading to the app.
#[derive(Default)]
pub struct GltfPlugin {
custom_vertex_attributes: HashMap<String, MeshVertexAttribute>,
custom_vertex_attributes: HashMap<Box<str>, MeshVertexAttribute>,
}

impl GltfPlugin {
Expand All @@ -40,8 +40,7 @@ impl GltfPlugin {
name: &str,
attribute: MeshVertexAttribute,
) -> Self {
self.custom_vertex_attributes
.insert(name.to_string(), attribute);
self.custom_vertex_attributes.insert(name.into(), attribute);
self
}
}
Expand Down Expand Up @@ -75,27 +74,27 @@ pub struct Gltf {
/// All scenes loaded from the glTF file.
pub scenes: Vec<Handle<Scene>>,
/// Named scenes loaded from the glTF file.
pub named_scenes: HashMap<String, Handle<Scene>>,
pub named_scenes: HashMap<Box<str>, Handle<Scene>>,
/// All meshes loaded from the glTF file.
pub meshes: Vec<Handle<GltfMesh>>,
/// Named meshes loaded from the glTF file.
pub named_meshes: HashMap<String, Handle<GltfMesh>>,
pub named_meshes: HashMap<Box<str>, Handle<GltfMesh>>,
/// All materials loaded from the glTF file.
pub materials: Vec<Handle<StandardMaterial>>,
/// Named materials loaded from the glTF file.
pub named_materials: HashMap<String, Handle<StandardMaterial>>,
pub named_materials: HashMap<Box<str>, Handle<StandardMaterial>>,
/// All nodes loaded from the glTF file.
pub nodes: Vec<Handle<GltfNode>>,
/// Named nodes loaded from the glTF file.
pub named_nodes: HashMap<String, Handle<GltfNode>>,
pub named_nodes: HashMap<Box<str>, Handle<GltfNode>>,
/// Default scene to be displayed.
pub default_scene: Option<Handle<Scene>>,
/// All animations loaded from the glTF file.
#[cfg(feature = "bevy_animation")]
pub animations: Vec<Handle<AnimationClip>>,
/// Named animations loaded from the glTF file.
#[cfg(feature = "bevy_animation")]
pub named_animations: HashMap<String, Handle<AnimationClip>>,
pub named_animations: HashMap<Box<str>, Handle<AnimationClip>>,
/// The gltf root of the gltf asset, see <https://docs.rs/gltf/latest/gltf/struct.Gltf.html>. Only has a value when `GltfLoaderSettings::include_source` is true.
pub source: Option<gltf::Gltf>,
}
Expand Down
Loading

0 comments on commit 09599f1

Please sign in to comment.