From da0235ae2698ee870c3a212ab6d257ef92bd66cc Mon Sep 17 00:00:00 2001 From: NathanW Date: Tue, 4 May 2021 14:23:06 -0600 Subject: [PATCH] [bevy_] Better SparseArray performance Problem: - bevy_ecs::SparseArray uses an `Option` internally to represent is the value is valid or not. - This causes extra branching as well as extra memory overhead due to Option's discriminatior. Solution: - Implement a `PackedOption` class which uses a value of `T` to represent the `None` state. - Have SparseArray use `PackedOption` internally over `Option`. --- crates/bevy_ecs/src/archetype.rs | 75 ++++++- crates/bevy_ecs/src/storage/sparse_set.rs | 56 ++++-- crates/bevy_ecs/src/world/entity_ref.rs | 17 +- crates/bevy_utils/src/lib.rs | 3 + crates/bevy_utils/src/packed_option.rs | 235 ++++++++++++++++++++++ 5 files changed, 351 insertions(+), 35 deletions(-) create mode 100644 crates/bevy_utils/src/packed_option.rs diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index 8e8e2702a413df..57a2d54286ba2b 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -4,6 +4,7 @@ use crate::{ entity::{Entity, EntityLocation}, storage::{Column, SparseArray, SparseSet, SparseSetIndex, TableId}, }; +use bevy_utils::{NoneValue, PackedOption}; use std::{ borrow::Cow, collections::HashMap, @@ -25,6 +26,11 @@ impl ArchetypeId { ArchetypeId(0) } + #[inline] + pub const fn invalid() -> ArchetypeId { + ArchetypeId(usize::MAX) + } + #[inline] pub const fn resource() -> ArchetypeId { ArchetypeId(1) @@ -46,11 +52,30 @@ pub struct AddBundle { pub bundle_status: Vec, } +impl NoneValue for ArchetypeId { + const NONE_VALUE: Self = ArchetypeId::invalid(); + + fn is_none_value(&self) -> bool { + *self == Self::NONE_VALUE + } +} + +impl NoneValue for AddBundle { + const NONE_VALUE: Self = Self { + archetype_id: ArchetypeId::NONE_VALUE, + bundle_status: Vec::new(), + }; + + fn is_none_value(&self) -> bool { + self.archetype_id.is_none_value() + } +} + #[derive(Default)] pub struct Edges { pub add_bundle: SparseArray, - pub remove_bundle: SparseArray>, - pub remove_bundle_intersection: SparseArray>, + pub remove_bundle: SparseArray, + pub remove_bundle_intersection: SparseArray, } impl Edges { @@ -76,28 +101,32 @@ impl Edges { } #[inline] - pub fn get_remove_bundle(&self, bundle_id: BundleId) -> Option> { - self.remove_bundle.get(bundle_id).cloned() + pub fn get_remove_bundle(&self, bundle_id: BundleId) -> PackedOption { + self.remove_bundle.get(bundle_id).cloned().into() } #[inline] - pub fn set_remove_bundle(&mut self, bundle_id: BundleId, archetype_id: Option) { + pub fn set_remove_bundle( + &mut self, + bundle_id: BundleId, + archetype_id: PackedOption, + ) { self.remove_bundle.insert(bundle_id, archetype_id); } #[inline] - pub fn get_remove_bundle_intersection( - &self, - bundle_id: BundleId, - ) -> Option> { - self.remove_bundle_intersection.get(bundle_id).cloned() + pub fn get_remove_bundle_intersection(&self, bundle_id: BundleId) -> PackedOption { + self.remove_bundle_intersection + .get(bundle_id) + .copied() + .into() } #[inline] pub fn set_remove_bundle_intersection( &mut self, bundle_id: BundleId, - archetype_id: Option, + archetype_id: PackedOption, ) { self.remove_bundle_intersection .insert(bundle_id, archetype_id); @@ -119,6 +148,17 @@ pub(crate) struct ArchetypeComponentInfo { pub(crate) archetype_component_id: ArchetypeComponentId, } +impl NoneValue for ArchetypeComponentInfo { + const NONE_VALUE: Self = Self { + storage_type: StorageType::Table, + archetype_component_id: ArchetypeComponentId::NONE_VALUE, + }; + + fn is_none_value(&self) -> bool { + self.archetype_component_id.is_none_value() + } +} + pub struct Archetype { id: ArchetypeId, entities: Vec, @@ -337,6 +377,11 @@ pub struct ArchetypeIdentity { pub struct ArchetypeComponentId(usize); impl ArchetypeComponentId { + #[inline] + pub const fn invalid() -> Self { + Self(usize::MAX) + } + #[inline] pub const fn new(index: usize) -> Self { Self(index) @@ -359,6 +404,14 @@ impl SparseSetIndex for ArchetypeComponentId { } } +impl NoneValue for ArchetypeComponentId { + const NONE_VALUE: Self = Self::invalid(); + + fn is_none_value(&self) -> bool { + *self == Self::invalid() + } +} + pub struct Archetypes { pub(crate) archetypes: Vec, pub(crate) archetype_component_count: usize, diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index a383b63364e3bd..e9d95da795c9df 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -3,15 +3,27 @@ use crate::{ entity::Entity, storage::BlobVec, }; +use bevy_utils::{NoneValue, PackedOption}; use std::{cell::UnsafeCell, marker::PhantomData}; -#[derive(Debug)] pub struct SparseArray { - values: Vec>, + values: Vec>, marker: PhantomData, } -impl Default for SparseArray { +impl std::fmt::Debug for SparseArray +where + V: NoneValue + std::fmt::Debug, +{ + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SparseArray") + .field("values", &self.values) + .field("marker", &self.marker) + .finish() + } +} + +impl Default for SparseArray { fn default() -> Self { Self::new() } @@ -27,7 +39,7 @@ impl SparseArray { } } -impl SparseArray { +impl SparseArray { pub fn with_capacity(capacity: usize) -> Self { Self { values: Vec::with_capacity(capacity), @@ -36,24 +48,30 @@ impl SparseArray { } #[inline] - pub fn insert(&mut self, index: I, value: V) { + pub fn insert>>(&mut self, index: I, value: T) { let index = index.sparse_set_index(); if index >= self.values.len() { - self.values.resize_with(index + 1, || None); + self.values.resize_with(index + 1, || None.into()); } - self.values[index] = Some(value); + self.values[index] = value.into(); } #[inline] pub fn contains(&self, index: I) -> bool { let index = index.sparse_set_index(); - self.values.get(index).map(|v| v.is_some()).unwrap_or(false) + self.values + .get(index) + .map(PackedOption::is_some) + .unwrap_or(false) } #[inline] pub fn get(&self, index: I) -> Option<&V> { let index = index.sparse_set_index(); - self.values.get(index).map(|v| v.as_ref()).unwrap_or(None) + self.values + .get(index) + .map(PackedOption::as_ref) + .unwrap_or(None) } #[inline] @@ -61,14 +79,17 @@ impl SparseArray { let index = index.sparse_set_index(); self.values .get_mut(index) - .map(|v| v.as_mut()) + .map(PackedOption::as_mut) .unwrap_or(None) } #[inline] - pub fn remove(&mut self, index: I) -> Option { + pub fn remove(&mut self, index: I) -> PackedOption { let index = index.sparse_set_index(); - self.values.get_mut(index).and_then(|value| value.take()) + self.values + .get_mut(index) + .map(PackedOption::take) + .unwrap_or_default() } #[inline] @@ -77,9 +98,9 @@ impl SparseArray { if index < self.values.len() { return self.values[index].get_or_insert_with(func); } - self.values.resize_with(index + 1, || None); + self.values.resize_with(index + 1, || None.into()); let value = &mut self.values[index]; - *value = Some(func()); + *value = func().into(); value.as_mut().unwrap() } } @@ -179,7 +200,7 @@ impl ComponentSparseSet { /// it exists). It is the caller's responsibility to drop the returned ptr (if Some is /// returned). pub fn remove_and_forget(&mut self, entity: Entity) -> Option<*mut u8> { - self.sparse.remove(entity).map(|dense_index| { + self.sparse.remove(entity).expand().map(|dense_index| { // SAFE: unique access to ticks unsafe { (*self.ticks.get()).swap_remove(dense_index); @@ -197,7 +218,7 @@ impl ComponentSparseSet { } pub fn remove(&mut self, entity: Entity) -> bool { - if let Some(dense_index) = self.sparse.remove(entity) { + if let Some(dense_index) = self.sparse.remove(entity).expand() { self.ticks.get_mut().swap_remove(dense_index); self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; @@ -233,6 +254,7 @@ impl Default for SparseSet { Self::new() } } + impl SparseSet { pub const fn new() -> Self { Self { @@ -338,7 +360,7 @@ impl SparseSet { } pub fn remove(&mut self, index: I) -> Option { - self.sparse.remove(index).map(|dense_index| { + self.sparse.remove(index).expand().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); diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index f8cb1dc60a183d..e2c39925feff4e 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -6,6 +6,7 @@ use crate::{ storage::{SparseSet, Storages}, world::{Mut, World}, }; +use bevy_utils::PackedOption; use std::any::TypeId; pub struct EntityRef<'w> { @@ -309,7 +310,8 @@ impl<'w> EntityMut<'w> { old_location.archetype_id, bundle_info, false, - )? + ) + .expand()? }; if new_archetype_id == old_location.archetype_id { @@ -767,7 +769,7 @@ unsafe fn remove_bundle_from_archetype( archetype_id: ArchetypeId, bundle_info: &BundleInfo, intersection: bool, -) -> Option { +) -> PackedOption { // check the archetype graph to see if the Bundle has been removed from this archetype in the // past let remove_bundle_result = { @@ -780,9 +782,10 @@ unsafe fn remove_bundle_from_archetype( current_archetype.edges().get_remove_bundle(bundle_info.id) } }; - let result = if let Some(result) = remove_bundle_result { + + let result = if remove_bundle_result.is_some() { // this Bundle removal result is cached. just return that! - result + remove_bundle_result } else { let mut next_table_components; let mut next_sparse_set_components; @@ -805,8 +808,8 @@ unsafe fn remove_bundle_from_archetype( // graph current_archetype .edges_mut() - .set_remove_bundle(bundle_info.id, None); - return None; + .set_remove_bundle(bundle_info.id, None.into()); + return None.into(); } } @@ -837,7 +840,7 @@ unsafe fn remove_bundle_from_archetype( next_table_components, next_sparse_set_components, ); - Some(new_archetype_id) + new_archetype_id.into() }; let current_archetype = &mut archetypes[archetype_id]; // cache the result in an edge diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index 9d66179df301ce..20843197d38e76 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -1,5 +1,8 @@ mod enum_variant_meta; +mod packed_option; + pub use enum_variant_meta::*; +pub use packed_option::*; pub use ahash::AHasher; pub use instant::{Duration, Instant}; diff --git a/crates/bevy_utils/src/packed_option.rs b/crates/bevy_utils/src/packed_option.rs new file mode 100644 index 00000000000000..39af08cf55f89e --- /dev/null +++ b/crates/bevy_utils/src/packed_option.rs @@ -0,0 +1,235 @@ +/// Types that have a reserved value which can't be created any other way. +pub trait NoneValue { + const NONE_VALUE: Self; + fn is_none_value(&self) -> bool; +} + +macro_rules! impl_max_none_value { + ($($ty:ty),+) => { + $(impl NoneValue for $ty { + const NONE_VALUE: Self = <$ty>::MAX; + + fn is_none_value(&self) -> bool { + *self == Self::NONE_VALUE + } + })* + }; +} + +impl_max_none_value!(usize); + +#[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Hash)] +pub struct PackedOption(T); + +impl PackedOption { + /// Returns `true` if the packed option is a `None` value. + #[inline] + pub fn is_none(&self) -> bool { + self.0.is_none_value() + } + + /// Returns `true` if the packed option is a `Some` value. + #[inline] + pub fn is_some(&self) -> bool { + !self.is_none() + } + + /// Expand the packed option into a normal `Option`. + #[inline] + pub fn expand(self) -> Option { + self.is_some().then(|| self.0) + } + + /// Unwrap a packed `Some` value or panic. + #[inline] + #[track_caller] + pub fn unwrap(self) -> T { + self.expand().unwrap() + } + + /// Unwrap a packed `Some` value or panic. + #[inline] + #[track_caller] + pub fn expect(self, msg: &str) -> T { + self.expand().expect(msg) + } + + /// Transform the packed option into a `Option<&T>` + #[inline] + pub fn as_ref(&self) -> Option<&T> { + self.is_some().then(|| &self.0) + } + + /// Transform the packed option into an `Option<&mut T>` + #[inline] + pub fn as_mut(&mut self) -> Option<&mut T> { + self.is_some().then(move || &mut self.0) + } + + /// TODO: Docs + #[inline] + pub fn get_or_insert_with T>(&mut self, f: F) -> &mut T { + if self.is_none() { + self.0 = f(); + } + &mut self.0 + } + + /// TODO: Docs + #[inline] + pub fn map U>(self, f: F) -> PackedOption { + if self.is_some() { + f(self.0).into() + } else { + None.into() + } + } + + pub fn take(&mut self) -> PackedOption { + std::mem::take(self) + } +} + +impl Default for PackedOption { + /// Create a default packed option representing `None`. + fn default() -> Self { + Self(::NONE_VALUE) + } +} + +impl From for PackedOption { + /// Convert `value` into a packed `Some(x)`. + fn from(value: T) -> Self { + debug_assert!( + !value.is_none_value(), + "Cannot make a `PackedOption` from the none value." + ); + Self(value) + } +} + +impl From> for PackedOption { + /// Convert an option into its packed equivalent. + fn from(opt: Option) -> Self { + match opt { + Some(x) => x.into(), + None => Self::default(), + } + } +} + +impl From> for Option { + fn from(packed: PackedOption) -> Self { + packed.expand() + } +} + +impl std::fmt::Debug for PackedOption +where + T: NoneValue + std::fmt::Debug, +{ + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + if self.is_none() { + write!(f, "None") + } else { + write!(f, "Some({:?})", self.0) + } + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[derive(Clone, Copy, Debug, Eq, PartialEq)] + struct TestType(u32); + + impl NoneValue for TestType { + const NONE_VALUE: Self = TestType(u32::MAX); + + fn is_none_value(&self) -> bool { + self.0 == u32::MAX + } + } + + #[test] + fn test_construction() { + // from option + let some: PackedOption = Some(TestType(1)).into(); + assert!(some.is_some()); + assert_eq!(some.unwrap(), TestType(1)); + + let none: PackedOption = None.into(); + assert!(none.is_none()); + + // from TestType + let some: PackedOption = TestType(1).into(); + assert!(some.is_some()); + assert_eq!(some.expect(""), TestType(1)); + } + + #[test] + #[should_panic] + fn test_unwrap_should_panic() { + std::panic::set_hook(Box::new(|_| {})); // prevents printing stack trace + let none: PackedOption = None.into(); + let _ = none.unwrap(); + } + + #[test] + #[should_panic(expected = "this should panic!")] + fn test_expect_should_panic() { + std::panic::set_hook(Box::new(|_| {})); + let none: PackedOption = None.into(); + let _ = none.expect("this should panic!"); + } + + #[test] + fn test_as_ref_and_mut() { + let mut some: PackedOption = TestType(1).into(); + + assert_eq!(some.as_ref().unwrap(), &TestType(1)); + assert_eq!(some.as_mut().unwrap(), &mut TestType(1)); + + let ref_mut = some.as_mut(); + *ref_mut.unwrap() = TestType(2); + + assert_eq!(some.unwrap(), TestType(2)); + } + + #[test] + fn test_get_or_insert_with() { + let mut some: PackedOption = TestType(1).into(); + assert_eq!(some.get_or_insert_with(|| TestType(2)), &mut TestType(1)); + + let mut none: PackedOption = None.into(); + assert_eq!(none.get_or_insert_with(|| TestType(2)), &mut TestType(2)); + } + + #[test] + fn test_map() { + let some: PackedOption = TestType(1).into(); + let some = some.map(|_| TestType(2)); + assert!(some.is_some()); + assert_eq!(some.unwrap(), TestType(2)); + + let none: PackedOption = None.into(); + let none = none.map(|_| TestType(2)); + assert!(none.is_none()); + } + + #[test] + fn test_take() { + let mut some: PackedOption = TestType(1).into(); + let taken = some.take(); + + assert!(some.is_none()); + assert!(taken.is_some()); + assert_eq!(taken.unwrap(), TestType(1)); + + let mut none: PackedOption = None.into(); + let taken = none.take(); + assert!(none.is_none()); + assert!(taken.is_none()); + } +}