From 57733bbec321b7fd0509fd4a7cd573e0f5858f48 Mon Sep 17 00:00:00 2001 From: James Liu Date: Sun, 3 Mar 2024 06:55:27 -0800 Subject: [PATCH] Use NonMaxUsize for non-component SparseSets (#12083) # Objective Adoption of #2104 and #11843. The `Option` wastes 3-7 bytes of memory per potential entry, and represents a scaling memory overhead as the ID space grows. The goal of this PR is to reduce memory usage without significantly impacting common use cases. Co-Authored By: @NathanSWard Co-Authored By: @tygyh ## Solution Replace `usize` in `SparseSet`'s sparse array with `nonmax::NonMaxUsize`. NonMaxUsize wraps a NonZeroUsize, and applies a bitwise NOT to the value when accessing it. This allows the compiler to niche the value and eliminate the extra padding used for the `Option` inside the sparse array, while moving the niche value from 0 to usize::MAX instead. Checking the [diff in x86 generated assembly](https://github.com/james7132/bevy_asm_tests/commit/6e4da653cc02afa0aad5a962a4ff50522df2bf54), this change actually results in fewer instructions generated. One potential downside is that it seems to have moved a load before a branch, which means we may be incurring a cache miss even if the element is not there. Note: unlike #2104 and #11843, this PR only targets the metadata stores for the ECS and not the component storage itself. Due to #9907 targeting `Entity::generation` instead of `Entity::index`, `ComponentSparseSet` storing only up to `u32::MAX` elements would become a correctness issue. This will come with a cost when inserting items into the SparseSet, as now there is a potential for a panic. These cost are really only incurred when constructing a new Table, Archetype, or Resource that has never been seen before by the World. All operations that are fairly cold and not on any particular hotpath, even for command application. --- ## Changelog Changed: `SparseSet` now can only store up to `usize::MAX - 1` elements instead of `usize::MAX`. Changed: `SparseSet` now uses 33-50% less memory overhead per stored item. --- crates/bevy_ecs/Cargo.toml | 1 + crates/bevy_ecs/src/storage/sparse_set.rs | 28 +++++++++++++---------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index b3372469694d0..db804278b4b2f 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -28,6 +28,7 @@ fixedbitset = "0.4.2" rustc-hash = "1.1" serde = "1" thiserror = "1.0" +nonmax = "0.5" [dev-dependencies] rand = "0.8" diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index dfb03f88681d2..dffdb71db961e 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -4,6 +4,7 @@ use crate::{ storage::{Column, TableRow}, }; use bevy_ptr::{OwningPtr, Ptr}; +use nonmax::NonMaxUsize; use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData}; type EntityIndex = u32; @@ -335,7 +336,7 @@ impl ComponentSparseSet { pub struct SparseSet { dense: Vec, indices: Vec, - sparse: SparseArray, + sparse: SparseArray, } /// A space-optimized version of [`SparseSet`] that cannot be changed @@ -344,7 +345,7 @@ pub struct SparseSet { pub(crate) struct ImmutableSparseSet { dense: Box<[V]>, indices: Box<[I]>, - sparse: ImmutableSparseArray, + sparse: ImmutableSparseArray, } macro_rules! impl_sparse_set { @@ -368,7 +369,7 @@ macro_rules! impl_sparse_set { pub fn get(&self, index: I) -> Option<&V> { self.sparse.get(index).map(|dense_index| { // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.get_unchecked(*dense_index) } + unsafe { self.dense.get_unchecked(dense_index.get()) } }) } @@ -379,7 +380,7 @@ macro_rules! impl_sparse_set { let dense = &mut self.dense; self.sparse.get(index).map(move |dense_index| { // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { dense.get_unchecked_mut(*dense_index) } + unsafe { dense.get_unchecked_mut(dense_index.get()) } }) } @@ -454,10 +455,11 @@ impl SparseSet { if let Some(dense_index) = self.sparse.get(index.clone()).cloned() { // SAFETY: dense indices stored in self.sparse always exist unsafe { - *self.dense.get_unchecked_mut(dense_index) = value; + *self.dense.get_unchecked_mut(dense_index.get()) = value; } } else { - self.sparse.insert(index.clone(), self.dense.len()); + self.sparse + .insert(index.clone(), NonMaxUsize::new(self.dense.len()).unwrap()); self.indices.push(index); self.dense.push(value); } @@ -468,11 +470,12 @@ impl SparseSet { pub fn get_or_insert_with(&mut self, index: I, func: impl FnOnce() -> V) -> &mut V { if let Some(dense_index) = self.sparse.get(index.clone()).cloned() { // SAFETY: dense indices stored in self.sparse always exist - unsafe { self.dense.get_unchecked_mut(dense_index) } + unsafe { self.dense.get_unchecked_mut(dense_index.get()) } } else { let value = func(); let dense_index = self.dense.len(); - self.sparse.insert(index.clone(), dense_index); + self.sparse + .insert(index.clone(), NonMaxUsize::new(dense_index).unwrap()); self.indices.push(index); self.dense.push(value); // SAFETY: dense index was just populated above @@ -491,11 +494,12 @@ impl SparseSet { /// Returns `None` if `index` does not have a value in the sparse set. pub fn remove(&mut self, index: I) -> Option { self.sparse.remove(index).map(|dense_index| { - let is_last = dense_index == self.dense.len() - 1; - let value = self.dense.swap_remove(dense_index); - self.indices.swap_remove(dense_index); + let index = dense_index.get(); + let is_last = index == self.dense.len() - 1; + let value = self.dense.swap_remove(index); + self.indices.swap_remove(index); if !is_last { - let swapped_index = self.indices[dense_index].clone(); + let swapped_index = self.indices[index].clone(); *self.sparse.get_mut(swapped_index).unwrap() = dense_index; } value