From 8f2909235c69379cba5ada1be3db9676bc13bc55 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sun, 24 Oct 2021 13:35:46 -0400 Subject: [PATCH 1/3] Document stronger safety requirements for implementing and using Join::get that should eliminate unsoundness if followed, add extra safety requirement to UnprotectedStorage::get implementation, comment out unsound methods on JoinIter and some Storages that were clearly unsound with the newly documented requirements (not all storages where reviewed) --- src/join/mod.rs | 22 ++++++++++++++--- src/lib.rs | 14 +++++------ src/storage/mod.rs | 54 ++++++++++++++++++++++++++++++++--------- src/storage/restrict.rs | 51 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 117 insertions(+), 24 deletions(-) diff --git a/src/join/mod.rs b/src/join/mod.rs index b9a4b8591..8054bf2b9 100644 --- a/src/join/mod.rs +++ b/src/join/mod.rs @@ -224,14 +224,24 @@ pub trait Join { /// then illegal memory access can occur. unsafe fn open(self) -> (Self::Mask, Self::Value); + // TODO: copy safety notes for callers to the impls of this method. + // TODO: also evaluate all impls /// Get a joined component value by a given index. /// /// # Safety /// /// * A call to `get` must be preceded by a check if `id` is part of - /// `Self::Mask` - /// * The implementation of this method may use unsafe code, but has no - /// invariants to meet + /// `Self::Mask`. + /// * The caller must ensure the lifetimes of mutable references returned from a call + /// of this method end before subsequent calls with the same `id`. + /// * Conversly, the implementation of the method must never create a mutable reference (even if it isn't + /// returned) that was returned by a previous call with a distinct `id`. Since there is no + /// requirement that the caller (if there was `JoinIter` would half to be a streaming + /// iterator). + /// are no guarantees that the caller will release + /// * The implementation of this method may use `unsafe` to extend the lifetime of returned references but + /// must ensure that any references within Self::Type cannot outlive the references + /// they were derived from in Self::Value. unsafe fn get(value: &mut Self::Value, id: Index) -> Self::Type; /// If this `Join` typically returns all indices in the mask, then iterating @@ -322,6 +332,10 @@ impl JoinIter { } impl JoinIter { + // TODO: these are unsound because they can be used to get two mutable references to the same + // component (in safe code) + + /* /// Allows getting joined values for specific entity. /// /// ## Example @@ -394,7 +408,7 @@ impl JoinIter { } else { None } - } + }*/ } impl std::iter::Iterator for JoinIter { diff --git a/src/lib.rs b/src/lib.rs index 972a840aa..b825778b5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,8 @@ #![warn(missing_docs)] -#![cfg_attr(feature = "nightly", feature(generic_associated_types, associated_type_defaults))] +#![cfg_attr( + feature = "nightly", + feature(generic_associated_types, associated_type_defaults) +)] //! # SPECS Parallel ECS //! @@ -207,9 +210,9 @@ pub mod world; pub use hibitset::BitSet; pub use shred::{ - Accessor, AccessorCow, BatchAccessor, BatchController, BatchUncheckedWorld, - Dispatcher, DispatcherBuilder, Read, ReadExpect, RunNow, - RunningTime, StaticAccessor, System, SystemData, World, Write, WriteExpect, + Accessor, AccessorCow, BatchAccessor, BatchController, BatchUncheckedWorld, Dispatcher, + DispatcherBuilder, Read, ReadExpect, RunNow, RunningTime, StaticAccessor, System, SystemData, + World, Write, WriteExpect, }; pub use shrev::ReaderId; @@ -230,6 +233,3 @@ pub use crate::{ }, world::{Builder, Component, Entities, Entity, EntityBuilder, LazyUpdate, WorldExt}, }; - -#[cfg(feature = "nightly")] -pub use crate::storage::DerefFlaggedStorage; \ No newline at end of file diff --git a/src/storage/mod.rs b/src/storage/mod.rs index 8e52304fb..c94b0640e 100644 --- a/src/storage/mod.rs +++ b/src/storage/mod.rs @@ -1,21 +1,32 @@ //! Component storage types, implementations for component joins, etc. +// TODO: This is unsound without a streaming JoinIter +// +// #[cfg(feature = "nightly")] +// pub use self::deref_flagged::{DerefFlaggedStorage, FlaggedAccessMut}; +// #[cfg(feature = "nightly")] +// mod deref_flagged; +// + +// TODO: Some parts of this involving mutable components are +// unsound without a streaming JoinIter. +// +// mod restrict; +// pub use self::restrict::{ +// ImmutableParallelRestriction, MutableParallelRestriction, PairedStorage, RestrictedStorage, +// SequentialRestriction, +// }; + pub use self::{ data::{ReadStorage, WriteStorage}, entry::{Entries, OccupiedEntry, StorageEntry, VacantEntry}, flagged::FlaggedStorage, generic::{GenericReadStorage, GenericWriteStorage}, - restrict::{ - ImmutableParallelRestriction, MutableParallelRestriction, RestrictedStorage, - SequentialRestriction, PairedStorage - }, storages::{ BTreeStorage, DefaultVecStorage, DenseVecStorage, HashMapStorage, NullStorage, VecStorage, }, track::{ComponentEvent, Tracked}, }; -#[cfg(feature = "nightly")] -pub use self::deref_flagged::{DerefFlaggedStorage, FlaggedAccessMut}; use self::storages::SliceAccess; @@ -42,10 +53,7 @@ mod data; mod drain; mod entry; mod flagged; -#[cfg(feature = "nightly")] -mod deref_flagged; mod generic; -mod restrict; mod storages; #[cfg(test)] mod tests; @@ -328,7 +336,7 @@ where } /// Tries to mutate the data associated with an `Entity`. - pub fn get_mut(&mut self, e: Entity) -> Option > { + pub fn get_mut(&mut self, e: Entity) -> Option> { if self.data.mask.contains(e.id()) && self.entities.is_alive(e) { // SAFETY: We checked the mask, so all invariants are met. Some(unsafe { self.data.inner.get_mut(e.id()) }) @@ -464,6 +472,23 @@ where // This is horribly unsafe. Unfortunately, Rust doesn't provide a way // to abstract mutable/immutable state at the moment, so we have to hack // our way through it. + + // # Safety + // + // See Join::get safety comments + // + // The caller is ensures any references returned are dropped before + // any future calls to this method with the same Index value. + // + // Additionally, UnprotectedStorage::get_mut is required to return distinct + // mutable references for distinct Index values and to not create references + // internally that would alias with other mutable references produced + // by calls with distinct Index values. Note, this isn't the same as `DistinctStorage` + // which is stricter in that it requires internally mutable operations never alias for + // distinct Index value (so it can be called in parallel). + // TODO: evaluate IdvStorage for this property (this is in a separate crate). + // + // Thus, this will not create aliased mutable references. let value: *mut Self::Value = v as *mut Self::Value; (*value).get_mut(i) } @@ -507,7 +532,8 @@ where pub trait UnprotectedStorage: TryDefault { /// The wrapper through with mutable access of a component is performed. #[cfg(feature = "nightly")] - type AccessMut<'a>: DerefMut where Self: 'a; + #[rustfmt::skip] + type AccessMut<'a>: DerefMut where Self: 'a; /// Clean the storage given a bitset with bits set for valid indices. /// Allows us to safely drop the storage. @@ -544,6 +570,9 @@ pub trait UnprotectedStorage: TryDefault { /// /// A mask should keep track of those states, and an `id` being contained /// in the tracking mask is sufficient to call this method. + /// + /// Implementations must not create references (even internally) that alias + /// references returned by previous calls with distinct `id` values. #[cfg(feature = "nightly")] unsafe fn get_mut(&mut self, id: Index) -> Self::AccessMut<'_>; @@ -558,6 +587,9 @@ pub trait UnprotectedStorage: TryDefault { /// /// A mask should keep track of those states, and an `id` being contained /// in the tracking mask is sufficient to call this method. + /// + /// Implementations must not create references (even internally) that alias + /// references returned by previous calls with distinct `id` values. #[cfg(not(feature = "nightly"))] unsafe fn get_mut(&mut self, id: Index) -> &mut T; diff --git a/src/storage/restrict.rs b/src/storage/restrict.rs index 99e4ccca5..1a7533cb0 100644 --- a/src/storage/restrict.rs +++ b/src/storage/restrict.rs @@ -12,7 +12,7 @@ use crate::join::Join; #[cfg(feature = "parallel")] use crate::join::ParJoin; use crate::{ - storage::{MaskedStorage, Storage, UnprotectedStorage, AccessMutReturn}, + storage::{AccessMutReturn, DistinctStorage, MaskedStorage, Storage, UnprotectedStorage}, world::{Component, EntitiesRes, Entity, Index}, }; @@ -86,6 +86,7 @@ where C: Component, S: BorrowMut + 'rf, B: Borrow + 'rf, + C::Storage: Sync + DistinctStorage, { } @@ -147,6 +148,48 @@ where } unsafe fn get(value: &mut Self::Value, id: Index) -> Self::Type { + // TODO: this is unsound with a non streaming JoinIter + // + // Calls with distinct `id`s will create aliased mutable + // references that are returned all referencing the same Self::Value. + // + // Additionally, using `get_mut` with two separate `PairedStorages`s can + // produce aliased mutable references: + // + // use specs::prelude::*; + // use specs::storage::PairedStorage; + // + // struct Pos(f32); + // + // impl Component for Pos { + // type Storage = VecStorage; + // } + // + // fn main() { + // let mut world = World::new(); + // + // world.register::(); + // + // world.create_entity().with(Pos(0.0)).build(); + // world.create_entity().with(Pos(1.6)).build(); + // world.create_entity().with(Pos(5.4)).build(); + // + // let entities = world.entities(); + // let mut pos = world.write_storage::(); + // + // let mut restrict_pos = pos.restrict_mut(); + // let mut vec: Vec> = (&mut restrict_pos).join().collect(); + // let (a, vec) = vec.split_first_mut().unwrap(); + // let (b, _) = vec.split_first_mut().unwrap(); + // let entity = entities.entity(0); + // let alias_a: &mut Pos = a.get_mut(entity).unwrap(); + // let alias_b: &mut Pos = b.get_mut(entity).unwrap(); + // assert!(core::ptr::eq(alias_a, alias_b)); + // } + // + // Also, since PairedStorage is Send, event if the underlying storage does not impl DistinctStorage + // sending a PairedStorage to another thread can be used to call the underlying get_mut + // simultaneously let value: &'rf mut Self::Value = &mut *(value as *mut Self::Value); PairedStorage { index: id, @@ -235,6 +278,10 @@ where { /// Gets the component related to the current entry without checking whether /// the storage has it or not. + // + // TODO: this is a misleading name since it implies something does need to be checked but we are + // skipping it, when instead this is just getting the one entity's component that can be + // retrieved without a check because we know it exists. pub fn get_unchecked(&self) -> &C { unsafe { self.storage.borrow().get(self.index) } } @@ -248,7 +295,7 @@ where { /// Gets the component related to the current entry without checking whether /// the storage has it or not. - pub fn get_mut_unchecked(&mut self) -> AccessMutReturn<'_, C> { + pub fn get_mut_unchecked(&mut self) -> AccessMutReturn<'_, C> { unsafe { self.storage.borrow_mut().get_mut(self.index) } } } From 290f5a8e62a3a053e4235a5ac6242cee741717e0 Mon Sep 17 00:00:00 2001 From: Imbris Date: Thu, 28 Oct 2021 19:09:03 -0400 Subject: [PATCH 2/3] Add new flagged storage that allows soundly defering tracked mutable access of a component in a non streaming join as well as using par_join with tracked mutable access --- src/changeset.rs | 2 +- src/lib.rs | 3 + src/storage/deref_flagged.rs | 12 +- src/storage/entry.rs | 12 +- src/storage/flagged.rs | 16 +- src/storage/generic.rs | 15 +- src/storage/mod.rs | 46 +++- src/storage/restrict.rs | 4 +- src/storage/split_flagged.rs | 482 +++++++++++++++++++++++++++++++++++ src/storage/storages.rs | 30 +++ 10 files changed, 598 insertions(+), 24 deletions(-) create mode 100644 src/storage/split_flagged.rs diff --git a/src/changeset.rs b/src/changeset.rs index 3cbbe9b7e..8b049f64d 100644 --- a/src/changeset.rs +++ b/src/changeset.rs @@ -126,7 +126,7 @@ impl<'a, T> Join for &'a mut ChangeSet { // exists yet. unsafe fn get(v: &mut Self::Value, id: Index) -> Self::Type { let value: *mut Self::Value = v as *mut Self::Value; - (*value).get_mut(id) + (*value).get_access_mut(id) } } diff --git a/src/lib.rs b/src/lib.rs index b825778b5..59a3e6826 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -233,3 +233,6 @@ pub use crate::{ }, world::{Builder, Component, Entities, Entity, EntityBuilder, LazyUpdate, WorldExt}, }; + +#[cfg(feature = "nightly")] +pub use crate::storage::UnsplitFlaggedStorage; diff --git a/src/storage/deref_flagged.rs b/src/storage/deref_flagged.rs index 5c9f8daf8..5bb84d654 100644 --- a/src/storage/deref_flagged.rs +++ b/src/storage/deref_flagged.rs @@ -55,6 +55,7 @@ where } impl> UnprotectedStorage for DerefFlaggedStorage { + #[rustfmt::skip] type AccessMut<'a> where T: 'a = FlaggedAccessMut<'a, >::AccessMut<'a>, C>; unsafe fn clean(&mut self, has: B) @@ -68,13 +69,20 @@ impl> UnprotectedStorage for DerefFlag self.storage.get(id) } - unsafe fn get_mut(&mut self, id: Index) -> Self::AccessMut<'_> { + unsafe fn get_mut(&mut self, id: Index) -> &mut C { + if self.emit_event() { + self.channel.single_write(ComponentEvent::Modified(id)); + } + self.storage.get_mut(id), + } + + unsafe fn get_access_mut(&mut self, id: Index) -> Self::AccessMut<'_> { let emit = self.emit_event(); FlaggedAccessMut { channel: &mut self.channel, emit, id, - access: self.storage.get_mut(id), + access: self.storage.get_access_mut(id), phantom: PhantomData, } } diff --git a/src/storage/entry.rs b/src/storage/entry.rs index 4c113c7b4..bf551d1ee 100644 --- a/src/storage/entry.rs +++ b/src/storage/entry.rs @@ -194,7 +194,7 @@ where pub fn get_mut(&mut self) -> AccessMutReturn<'_, T> { // SAFETY: This is safe since `OccupiedEntry` is only constructed // after checking the mask. - unsafe { self.storage.data.inner.get_mut(self.id) } + unsafe { self.storage.data.inner.get_access_mut(self.id) } } /// Converts the `OccupiedEntry` into a mutable reference bounded by @@ -202,12 +202,16 @@ where pub fn into_mut(self) -> AccessMutReturn<'a, T> { // SAFETY: This is safe since `OccupiedEntry` is only constructed // after checking the mask. - unsafe { self.storage.data.inner.get_mut(self.id) } + unsafe { self.storage.data.inner.get_access_mut(self.id) } } /// Inserts a value into the storage and returns the old one. pub fn insert(&mut self, mut component: T) -> T { - std::mem::swap(&mut component, self.get_mut().deref_mut()); + // SAFETY: This is safe since `OccupiedEntry` is only constructed + // after checking the mask. + std::mem::swap(&mut component, unsafe { + self.storage.data.inner.get_mut(self.id) + }); component } @@ -235,7 +239,7 @@ where // SAFETY: This is safe since we added `self.id` to the mask. unsafe { self.storage.data.inner.insert(self.id, component); - self.storage.data.inner.get_mut(self.id) + self.storage.data.inner.get_access_mut(self.id) } } } diff --git a/src/storage/flagged.rs b/src/storage/flagged.rs index 3d2fe53aa..bff0a06a7 100644 --- a/src/storage/flagged.rs +++ b/src/storage/flagged.rs @@ -201,6 +201,7 @@ where impl> UnprotectedStorage for FlaggedStorage { #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'a> where T: 'a = >::AccessMut<'a>; unsafe fn clean(&mut self, has: B) @@ -214,20 +215,27 @@ impl> UnprotectedStorage for FlaggedSt self.storage.get(id) } - #[cfg(feature = "nightly")] - unsafe fn get_mut(&mut self, id: Index) -> >::AccessMut<'_> { + unsafe fn get_mut(&mut self, id: Index) -> &mut C { if self.emit_event() { self.channel.single_write(ComponentEvent::Modified(id)); } self.storage.get_mut(id) } + #[cfg(feature = "nightly")] + unsafe fn get_access_mut(&mut self, id: Index) -> >::AccessMut<'_> { + if self.emit_event() { + self.channel.single_write(ComponentEvent::Modified(id)); + } + self.storage.get_access_mut(id) + } + #[cfg(not(feature = "nightly"))] - unsafe fn get_mut(&mut self, id: Index) -> &mut C { + unsafe fn get_access_mut(&mut self, id: Index) -> &mut C { if self.emit_event() { self.channel.single_write(ComponentEvent::Modified(id)); } - self.storage.get_mut(id) + self.storage.get_access_mut(id) } unsafe fn insert(&mut self, id: Index, comp: C) { diff --git a/src/storage/generic.rs b/src/storage/generic.rs index 1f38ac0cc..7458a2290 100644 --- a/src/storage/generic.rs +++ b/src/storage/generic.rs @@ -1,9 +1,8 @@ -#[cfg(feature = "nightly")] -use std::ops::DerefMut; use crate::{ - storage::{InsertResult, ReadStorage, WriteStorage, AccessMutReturn}, + storage::{AccessMutReturn, InsertResult, ReadStorage, WriteStorage}, world::{Component, Entity}, }; + #[cfg(feature = "nightly")] use crate::storage::UnprotectedStorage; @@ -88,7 +87,8 @@ pub trait GenericWriteStorage { type Component: Component; /// The wrapper through with mutable access of a component is performed. #[cfg(feature = "nightly")] - type AccessMut<'a>: DerefMut where Self: 'a; + #[rustfmt::skip] + type AccessMut<'a> where Self: 'a; /// Get mutable access to an `Entity`s component fn get_mut(&mut self, entity: Entity) -> Option>; @@ -97,7 +97,10 @@ pub trait GenericWriteStorage { /// exist, it is automatically created using `Default::default()`. /// /// Returns None if the entity is dead. - fn get_mut_or_default(&mut self, entity: Entity) -> Option> + fn get_mut_or_default( + &mut self, + entity: Entity, + ) -> Option> where Self::Component: Default; @@ -117,6 +120,7 @@ where { type Component = T; #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'b> where Self: 'b = <::Storage as UnprotectedStorage>::AccessMut<'b>; fn get_mut(&mut self, entity: Entity) -> Option> { @@ -155,6 +159,7 @@ where { type Component = T; #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'c> where Self: 'c = <::Storage as UnprotectedStorage>::AccessMut<'c>; fn get_mut(&mut self, entity: Entity) -> Option> { diff --git a/src/storage/mod.rs b/src/storage/mod.rs index c94b0640e..fc54f9e67 100644 --- a/src/storage/mod.rs +++ b/src/storage/mod.rs @@ -17,6 +17,14 @@ // SequentialRestriction, // }; +#[cfg(feature = "nightly")] +pub use self::split_flagged::{ + EventCollector, EventPool, EventSink, FlaggedAccessMut, SplitFlaggedAccessMut, + SplitFlaggedStorage, UnsplitFlaggedStorage, +}; +#[cfg(feature = "nightly")] +mod split_flagged; + pub use self::{ data::{ReadStorage, WriteStorage}, entry::{Entries, OccupiedEntry, StorageEntry, VacantEntry}, @@ -339,7 +347,7 @@ where pub fn get_mut(&mut self, e: Entity) -> Option> { if self.data.mask.contains(e.id()) && self.entities.is_alive(e) { // SAFETY: We checked the mask, so all invariants are met. - Some(unsafe { self.data.inner.get_mut(e.id()) }) + Some(unsafe { self.data.inner.get_access_mut(e.id()) }) } else { None } @@ -356,7 +364,7 @@ where let id = e.id(); if self.data.mask.contains(id) { // SAFETY: We checked the mask, so all invariants are met. - std::mem::swap(&mut v, unsafe { self.data.inner.get_mut(id).deref_mut() }); + std::mem::swap(&mut v, unsafe { self.data.inner.get_mut(id) }); Ok(Some(v)) } else { self.data.mask.add(id); @@ -490,7 +498,7 @@ where // // Thus, this will not create aliased mutable references. let value: *mut Self::Value = v as *mut Self::Value; - (*value).get_mut(i) + (*value).get_access_mut(i) } } @@ -533,7 +541,7 @@ pub trait UnprotectedStorage: TryDefault { /// The wrapper through with mutable access of a component is performed. #[cfg(feature = "nightly")] #[rustfmt::skip] - type AccessMut<'a>: DerefMut where Self: 'a; + type AccessMut<'a> where Self: 'a; /// Clean the storage given a bitset with bits set for valid indices. /// Allows us to safely drop the storage. @@ -573,13 +581,39 @@ pub trait UnprotectedStorage: TryDefault { /// /// Implementations must not create references (even internally) that alias /// references returned by previous calls with distinct `id` values. + unsafe fn get_mut(&mut self, id: Index) -> &mut T; + + /// Tries mutating the data associated with an `Index`. + /// This is unsafe because the external set used + /// to protect this storage is absent. + /// + /// Unlike, [UnprotectedStorage::get_mut] this method allows the storage + /// to wrap the mutable reference (when the `nightly` feature is enabled) to allow + /// any logic such as emitting modification events to only occur if the component + /// is mutated. + /// + /// # Safety + /// + /// May only be called after a call to `insert` with `id` and + /// no following call to `remove` with `id`. + /// + /// A mask should keep track of those states, and an `id` being contained + /// in the tracking mask is sufficient to call this method. + /// + /// Implementations must not create references (even internally) that alias + /// references returned by previous calls with distinct `id` values. #[cfg(feature = "nightly")] - unsafe fn get_mut(&mut self, id: Index) -> Self::AccessMut<'_>; + unsafe fn get_access_mut(&mut self, id: Index) -> Self::AccessMut<'_>; /// Tries mutating the data associated with an `Index`. /// This is unsafe because the external set used /// to protect this storage is absent. /// + /// Unlike, [UnprotectedStorage::get_mut] this method allows the storage + /// to wrap the mutable reference (when the `nightly` feature is enabled) to allow + /// any logic such as emitting modification events to only occur if the component + /// is mutated. + /// /// # Safety /// /// May only be called after a call to `insert` with `id` and @@ -591,7 +625,7 @@ pub trait UnprotectedStorage: TryDefault { /// Implementations must not create references (even internally) that alias /// references returned by previous calls with distinct `id` values. #[cfg(not(feature = "nightly"))] - unsafe fn get_mut(&mut self, id: Index) -> &mut T; + unsafe fn get_access_mut(&mut self, id: Index) -> &mut T; /// Inserts new data for a given `Index`. /// diff --git a/src/storage/restrict.rs b/src/storage/restrict.rs index 1a7533cb0..adbd28c9e 100644 --- a/src/storage/restrict.rs +++ b/src/storage/restrict.rs @@ -296,7 +296,7 @@ where /// Gets the component related to the current entry without checking whether /// the storage has it or not. pub fn get_mut_unchecked(&mut self) -> AccessMutReturn<'_, C> { - unsafe { self.storage.borrow_mut().get_mut(self.index) } + unsafe { self.storage.borrow_mut().get_access_mut(self.index) } } } @@ -338,7 +338,7 @@ where /// threads. pub fn get_mut(&mut self, entity: Entity) -> Option> { if self.bitset.borrow().contains(entity.id()) && self.entities.is_alive(entity) { - Some(unsafe { self.storage.borrow_mut().get_mut(entity.id()) }) + Some(unsafe { self.storage.borrow_mut().get_access_mut(entity.id()) }) } else { None } diff --git a/src/storage/split_flagged.rs b/src/storage/split_flagged.rs new file mode 100644 index 000000000..33bf7f786 --- /dev/null +++ b/src/storage/split_flagged.rs @@ -0,0 +1,482 @@ +use std::{ + marker::PhantomData, + ops::{Deref, DerefMut}, + sync::Mutex, +}; + +use hibitset::BitSetLike; + +#[cfg(feature = "parallel")] +use crate::join::ParJoin; +use crate::{ + join::Join, + storage::{ + ComponentEvent, DenseVecStorage, DistinctStorage, MaskedStorage, Storage, Tracked, + TryDefault, UnprotectedStorage, + }, + world::{Component, Entity, Index}, +}; + +use hibitset::BitSet; +use shrev::EventChannel; + +/// Wrapper storage that tracks modifications, insertions, and removals of +/// components through an `EventChannel`, in a similar manner to `FlaggedStorage`. +/// +/// Unlike `FlaggedStorage`, this storage uses a wrapper type for mutable +/// accesses that only emits modification events when the component is explicitly +/// accessed mutably which. This means that simply performing a mutable join will +/// not, by itself, trigger a modification event. +/// +/// To use this storage in a mutable join it should first be split into an [SplitFlaggedStorage] +/// and an [EventPool] using [Storage::split]. This is because the deferred mutable +/// access through the wrapper type requires passing in the channel separately. This strategy is +/// neccessary for soundness because [JoinIter] is not a streaming iterator so if we were to place +/// a mutable reference to the event channel inside of the mutable component wrapper this will +/// cause aliasing of mutable references. In addition, this strategy enables us to safely perform +/// parallel mutable joins with components in this storage. +/// +/// Note: no effort is made to ensure a particular ordering of the modification +/// events that occur withing the scope of a single `split`. +pub struct UnsplitFlaggedStorage> { + channel: EventChannel, + storage: T, + #[cfg(feature = "storage-event-control")] + event_emission: bool, + phantom: PhantomData, +} + +/// Pool of component modification events. +/// +/// In the single threaded `.join()` case this can be used directly for tracked +/// access of mutable components through [SplitFlaggedAccessMut]. For the parrallel +/// `.par_join()` a collector for each rayon job can be created using [Self::collector]. +/// +/// If you leak this (e.g. with `mem::forget`) the events will never be sent. +pub struct EventPool<'a, C> { + channel: &'a mut EventChannel, + pool: Mutex>>, + phantom: PhantomData, +} + +/// If you leak this (e.g. with `mem::forget`) the events will never be sent. +pub struct EventCollector<'a, C> { + pool: &'a Mutex>>, + pending_events: Vec, + phantom: PhantomData, +} + +impl EventPool<'_, C> { + /// Create a new `EventCollector` for collecting events + /// This method can be called in the init closure of rayon methods like + /// `for_each_init` in order to produce a collector as needed for each rayon + /// job and collect component modification events in parallel. + pub fn collector(&self) -> EventCollector<'_, C> { + EventCollector { + pool: &self.pool, + pending_events: Vec::new(), + phantom: PhantomData, + } + } +} + +impl<'a, C> Drop for EventPool<'a, C> { + fn drop(&mut self) { + let event_pool = core::mem::take(self.pool.get_mut().unwrap_or_else(|e| e.into_inner())); + // Send events to through channel + event_pool.into_iter().for_each(|events| { + self.channel + .iter_write(events.into_iter().map(|id| ComponentEvent::Modified(id))); + }) + } +} + +impl<'a, C> Drop for EventCollector<'a, C> { + fn drop(&mut self) { + let events = core::mem::take(&mut self.pending_events); + // Ignore poison + let mut guard = self.pool.lock().unwrap_or_else(|e| e.into_inner()); + // Add locally collected events to the pool + guard.push(events); + } +} + +/// Abstracts over [EventPool] and [EventCollector] so they can both be used for +/// tracked access of components to reduce complexity of the single threaded `.join()` +/// case. +/// +/// This trait cannot be implemented for types outside this crate. Actual methods +/// are in a private super trait. +pub trait EventSink: private::EventSink {} // TODO: EventCollector seems like a better name but that overlaps with the type named that :/ + +impl EventSink for T where T: private::EventSink {} + +// https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed +mod private { + pub trait EventSink { + fn vec_mut(&mut self) -> &mut Vec; + } +} + +impl private::EventSink for EventPool<'_, C> { + fn vec_mut(&mut self) -> &mut Vec { + // Reach into mutex ignoring poison + let event_pool = self.pool.get_mut().unwrap_or_else(|e| e.into_inner()); + + // If the vec of vecs is empty add one + if event_pool.is_empty() { + event_pool.push(Vec::new()); + } + + // Return mutable reference to the first vec + &mut event_pool[0] + } +} + +impl private::EventSink for EventCollector<'_, C> { + fn vec_mut(&mut self) -> &mut Vec { + &mut self.pending_events + } +} + +pub struct SplitFlaggedStorage<'a, C, T> { + // Deconstruction of Storage so we can impl Join/ParJoin + // + // struct MaskedStorage + // mask: BitSet, + // inner: T::Storage, + // + // struct Storage<'e, T, D> + // data: D, // MaskedStorage + // entities: Fetch<'e, EntitiesRes>, + // phantom: PhantomData, + mask: &'a BitSet, + storage: &'a mut T, + + #[cfg(feature = "storage-event-control")] + event_emission: bool, + // Invariant lifetime brand + phantom: PhantomData, +} + +impl<'e, C, D, T> Storage<'e, C, D> +where + C: Component>, + D: DerefMut>, + T: UnprotectedStorage, +{ + /// Temporarily divide into a [SplitFlaggedStorage] which borrows the underlying + /// storage and an [EventPool] which borrows the underlying event channel and + /// is used to collect modification events. This allows deferred mutable access of components + /// in mutable. + pub fn split(&mut self) -> (SplitFlaggedStorage<'_, C, T>, EventPool<'_, C>) { + let masked_storage = self.data.deref_mut(); + let split_storage = SplitFlaggedStorage { + mask: &masked_storage.mask, + storage: &mut masked_storage.inner.storage, + + // TODO: add method on SplitFlaggedStorage to toggle this? + #[cfg(feature = "storage-event-control")] + event_emission: self.event_emission, + phantom: PhantomData, + }; + + let pool = EventPool { + channel: &mut masked_storage.inner.channel, + pool: Mutex::new(Vec::new()), + phantom: PhantomData, + }; + + (split_storage, pool) + } +} + +impl> UnsplitFlaggedStorage { + /// Access the data associated with an `Index` for mutation. Internally emits + /// a component modification event if event emission is not disabled for this + /// storage. + /// + /// # Safety: + /// + /// Same requirements as [UnprotectedStorage::get_mut] + pub unsafe fn get_mut_tracked(&mut self, id: Index) -> T::AccessMut<'_> { + if self.emit_event() { + self.channel.single_write(ComponentEvent::Modified(id)); + } + self.storage.get_access_mut(id) + } +} + +impl<'e, T, D, I> Storage<'e, T, D> +where + T: Component>, + D: DerefMut>, + I: UnprotectedStorage, +{ + /// Tries to mutate the data associated with an `Entity`. Internally emits + /// a component modification event if event emission is not disabled for this + /// storage. Conveinence method for [UnsplitFlaggedStorage] which normally requires + /// .... TODO: describe this when details crystallize. + pub fn get_mut_tracked(&mut self, e: Entity) -> Option> { + if self.data.mask.contains(e.id()) && self.entities.is_alive(e) { + // SAFETY: We checked the mask, so all invariants are met. + Some(unsafe { self.data.inner.get_mut_tracked(e.id()) }) + } else { + None + } + } +} + +impl UnsplitFlaggedStorage { + #[cfg(feature = "storage-event-control")] + fn emit_event(&self) -> bool { + self.event_emission + } + + #[cfg(not(feature = "storage-event-control"))] + fn emit_event(&self) -> bool { + true + } +} + +impl Default for UnsplitFlaggedStorage +where + T: TryDefault, +{ + fn default() -> Self { + Self { + channel: EventChannel::::default(), + storage: T::unwrap_default(), + #[cfg(feature = "storage-event-control")] + event_emission: true, + phantom: PhantomData, + } + } +} + +impl> UnprotectedStorage for UnsplitFlaggedStorage { + // UnsplitFlaggedStorage is meant to be split into SplitFlaggedStorage and a EventCollector + // before mutable joins so the AccessMut type for the unsplit storage that is + // the unit type + // TODO: Ideally Self wouldn't be able to be mutably joined over at all + #[rustfmt::skip] + type AccessMut<'a> where T: 'a = (); + + unsafe fn clean(&mut self, has: B) + where + B: BitSetLike, + { + self.storage.clean(has); + } + + unsafe fn get(&self, id: Index) -> &C { + self.storage.get(id) + } + + unsafe fn get_mut(&mut self, id: Index) -> &mut C { + if self.emit_event() { + self.channel.single_write(ComponentEvent::Modified(id)); + } + + self.storage.get_mut(id) + } + + unsafe fn get_access_mut(&mut self, _id: Index) -> Self::AccessMut<'_> {} + + unsafe fn insert(&mut self, id: Index, comp: C) { + if self.emit_event() { + self.channel.single_write(ComponentEvent::Inserted(id)); + } + self.storage.insert(id, comp); + } + + unsafe fn remove(&mut self, id: Index) -> C { + if self.emit_event() { + self.channel.single_write(ComponentEvent::Removed(id)); + } + self.storage.remove(id) + } +} + +impl Tracked for UnsplitFlaggedStorage { + fn channel(&self) -> &EventChannel { + &self.channel + } + + fn channel_mut(&mut self) -> &mut EventChannel { + &mut self.channel + } + + #[cfg(feature = "storage-event-control")] + fn set_event_emission(&mut self, emit: bool) { + self.event_emission = emit; + } + + #[cfg(feature = "storage-event-control")] + fn event_emission(&self) -> bool { + self.event_emission + } +} + +impl<'a, C, T> SplitFlaggedStorage<'a, C, T> { + #[cfg(feature = "storage-event-control")] + fn emit_event(&self) -> bool { + self.event_emission + } + + #[cfg(not(feature = "storage-event-control"))] + fn emit_event(&self) -> bool { + true + } +} + +impl<'a, 'e, C, T> Join for &'a mut SplitFlaggedStorage<'e, C, T> +where + C: Component, + T: UnprotectedStorage, +{ + type Mask = &'a BitSet; + type Type = SplitFlaggedAccessMut<>::AccessMut<'a>, C>; + type Value = (&'a mut T, bool); // bool is event_emission + + // SAFETY: No unsafe code and no invariants to fulfill. + unsafe fn open(self) -> (Self::Mask, Self::Value) { + let event_emission = self.emit_event(); + (self.mask, (self.storage, event_emission)) + } + + unsafe fn get(v: &mut Self::Value, id: Index) -> Self::Type { + // Note: comments copy-pasted from Storage Join impl + + // This is horribly unsafe. Unfortunately, Rust doesn't provide a way + // to abstract mutable/immutable state at the moment, so we have to hack + // our way through it. + + // # Safety + // + // See Join::get safety comments + // + // The caller is ensures any references returned are dropped before + // any future calls to this method with the same Index value. + // + // Additionally, UnprotectedStorage::get_mut is required to return distinct + // mutable references for distinct Index values and to not create references + // internally that would alias with other mutable references produced + // by calls with distinct Index values. Note, this isn't the same as `DistinctStorage` + // which is stricter in that it requires internally mutable operations never alias for + // distinct Index value (so it can be called in parallel). + // + // Thus, this will not create aliased mutable references. + let (storage, event_emission) = v; + let storage: *mut T = *storage as *mut T; + let access = (*storage).get_access_mut(id); + SplitFlaggedAccessMut { + emit: *event_emission, + id, + access, + phantom: PhantomData, + } + } +} + +// SAFETY: This is safe because of the `DistinctStorage` guarantees. +#[cfg(feature = "parallel")] +unsafe impl<'a, 'e, C, T> ParJoin for &'a mut SplitFlaggedStorage<'e, C, T> +where + C: Component, + T: UnprotectedStorage, + T: Sync + DistinctStorage, +{ +} + +pub struct SplitFlaggedAccessMut { + emit: bool, + id: Index, + access: A, + phantom: PhantomData, +} + +impl Deref for SplitFlaggedAccessMut { + type Target = A; + fn deref(&self) -> &Self::Target { + &self.access + } +} + +impl SplitFlaggedAccessMut { + /// Provides mutable access and emits a modification event. + /// + /// Note: Requiring the event collector/pool to have the same component type C doesn't + /// guarantee completely that the [EventCollector]/[EventPool] and the [FlaggedAccessMut] + /// are associated with the same source [UnsplitFlaggedStorage] but this does generally + /// provide helpful resistance to misuse in most cases dealing with a single ecs world. + /// If we want to guarantee this relation lifetime branding could be used instead. + pub fn access>(&mut self, event_collector: &mut S) -> &mut A { + if self.emit { + event_collector.vec_mut().push(self.id); + } + &mut self.access + } + + /// Like [Self::access] but skips emitting a modification event. + pub fn access_untracked(&mut self) -> &mut A { + &mut self.access + } + + /// If you are going to be passing this around a lot it can be useful to combine it with + /// the event collector so only one thing needs to be passed around for each wrapped component. + /// + /// Note: Requiring the event collector/pool to have the same component type C doesn't + /// guarantee completely that the [EventCollector]/[EventPool] and the [FlaggedAccessMut] + /// are associated with the same source [UnsplitFlaggedStorage] but this does generally + /// provide helpful resistance to misuse in most cases dealing with a single ecs world. + /// If we want to guarantee this relation lifetime branding could be used instead. + pub fn with_collector>( + self, + event_collector: &mut S, + ) -> FlaggedAccessMut<'_, A, C> { + FlaggedAccessMut { + emit: self.emit, + id: self.id, + access: self.access, + phantom: self.phantom, + events: event_collector.vec_mut(), + } + } +} + +pub struct FlaggedAccessMut<'a, A, C> { + emit: bool, + id: Index, + access: A, + phantom: PhantomData, + events: &'a mut Vec, +} + +impl Deref for FlaggedAccessMut<'_, A, C> { + type Target = A; + fn deref(&self) -> &Self::Target { + &self.access + } +} + +impl<'a, A, C> FlaggedAccessMut<'a, A, C> { + /// Provides mutable access and emits a modification event. + /// + /// Note: Requiring the event collector/pool to have the same component type C doesn't + /// guarantee completely that the [EventCollector]/[EventPool] and the [FlaggedAccessMut] + /// are associated with the same source [UnsplitFlaggedStorage] but this does generally + /// provide helpful resistance to misuse in most cases dealing with a single ecs world. + /// If we want to guarantee this relation lifetime branding could be used instead. + pub fn access>(&mut self) -> &mut A { + if self.emit { + self.events.push(self.id); + } + &mut self.access + } + + /// Like [Self::access] but skips emitting a modification event. + pub fn access_untracked(&mut self) -> &mut A { + &mut self.access + } +} diff --git a/src/storage/storages.rs b/src/storage/storages.rs index 27417ec1e..baa11eb7f 100644 --- a/src/storage/storages.rs +++ b/src/storage/storages.rs @@ -33,6 +33,7 @@ impl Default for BTreeStorage { impl UnprotectedStorage for BTreeStorage { #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'a> where T: 'a = &'a mut T; unsafe fn clean(&mut self, _has: B) @@ -50,6 +51,10 @@ impl UnprotectedStorage for BTreeStorage { self.0.get_mut(&id).unwrap() } + unsafe fn get_access_mut(&mut self, id: Index) -> &mut T { + self.get_mut(id) + } + unsafe fn insert(&mut self, id: Index, v: T) { self.0.insert(id, v); } @@ -74,6 +79,7 @@ impl Default for HashMapStorage { impl UnprotectedStorage for HashMapStorage { #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'a> where T: 'a = &'a mut T; unsafe fn clean(&mut self, _has: B) @@ -91,6 +97,10 @@ impl UnprotectedStorage for HashMapStorage { self.0.get_mut(&id).unwrap() } + unsafe fn get_access_mut(&mut self, id: Index) -> &mut T { + self.get_mut(id) + } + unsafe fn insert(&mut self, id: Index, v: T) { self.0.insert(id, v); } @@ -154,6 +164,7 @@ impl SliceAccess for DenseVecStorage { impl UnprotectedStorage for DenseVecStorage { #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'a> where T: 'a = &'a mut T; unsafe fn clean(&mut self, _has: B) @@ -173,6 +184,10 @@ impl UnprotectedStorage for DenseVecStorage { self.data.get_unchecked_mut(did as usize) } + unsafe fn get_access_mut(&mut self, id: Index) -> &mut T { + self.get_mut(id) + } + unsafe fn insert(&mut self, id: Index, v: T) { let id = id as usize; if self.data_id.len() <= id { @@ -211,6 +226,7 @@ where T: Default, { #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'a> where T: 'a = &'a mut T; unsafe fn clean(&mut self, _has: B) @@ -227,6 +243,10 @@ where &mut self.0 } + unsafe fn get_access_mut(&mut self, id: Index) -> &mut T { + self.get_mut(id) + } + unsafe fn insert(&mut self, _: Index, _: T) {} unsafe fn remove(&mut self, _: Index) -> T { @@ -281,6 +301,7 @@ impl SliceAccess for VecStorage { impl UnprotectedStorage for VecStorage { #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'a> where T: 'a = &'a mut T; unsafe fn clean(&mut self, has: B) @@ -305,6 +326,10 @@ impl UnprotectedStorage for VecStorage { &mut *self.0.get_unchecked_mut(id as usize).as_mut_ptr() } + unsafe fn get_access_mut(&mut self, id: Index) -> &mut T { + self.get_mut(id) + } + unsafe fn insert(&mut self, id: Index, v: T) { let id = id as usize; if self.0.len() <= id { @@ -346,6 +371,7 @@ where T: Default, { #[cfg(feature = "nightly")] + #[rustfmt::skip] type AccessMut<'a> where T: 'a = &'a mut T; unsafe fn clean(&mut self, _has: B) @@ -363,6 +389,10 @@ where self.0.get_unchecked_mut(id as usize) } + unsafe fn get_access_mut(&mut self, id: Index) -> &mut T { + self.get_mut(id) + } + unsafe fn insert(&mut self, id: Index, v: T) { let id = id as usize; From 6eac447270b53860e8d5529b983f53c570892d39 Mon Sep 17 00:00:00 2001 From: Imbris Date: Fri, 29 Oct 2021 00:17:03 -0400 Subject: [PATCH 3/3] Improve the docs with respect to the new flagged storage variant and the current removal of restricted storage (and comment out an example that was using the restricted storage) --- Cargo.toml | 5 +++-- src/storage/flagged.rs | 3 +++ src/storage/split_flagged.rs | 18 +++++++++++------- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index cd25150f4..40cc2ff58 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,8 +71,9 @@ name = "cluster_bomb" [[example]] name = "bitset" -[[example]] -name = "track" +# TODO: restricted storage is unsound without streaming iterator or some changes made to it (i.e. to not allow access to component data on other entities in the mutable case) +# [[example]] +# name = "track" [[example]] name = "ordered_track" diff --git a/src/storage/flagged.rs b/src/storage/flagged.rs index bff0a06a7..62a69b61a 100644 --- a/src/storage/flagged.rs +++ b/src/storage/flagged.rs @@ -15,6 +15,9 @@ use shrev::EventChannel; /// **Note:** Joining over all components of a `FlaggedStorage` /// mutably will flag all components. /// +/// **Note:** restricted storages are currently removed since they need some changes to be sound so +/// the below advice won't currently work. Use `UnsplitFlaggedStorage` instead. +/// /// What you want to instead is to use `restrict_mut()` to first /// get the entities which contain the component and then conditionally /// modify the component after a call to `get_mut_unchecked()` or `get_mut()`. diff --git a/src/storage/split_flagged.rs b/src/storage/split_flagged.rs index 33bf7f786..52fb30243 100644 --- a/src/storage/split_flagged.rs +++ b/src/storage/split_flagged.rs @@ -21,9 +21,10 @@ use hibitset::BitSet; use shrev::EventChannel; /// Wrapper storage that tracks modifications, insertions, and removals of -/// components through an `EventChannel`, in a similar manner to `FlaggedStorage`. +/// components through an `EventChannel`, in a similar manner to +/// [FlaggedStorage](crate::FlaggedStorage). /// -/// Unlike `FlaggedStorage`, this storage uses a wrapper type for mutable +/// Unlike [FlaggedStorage](crate::FlaggedStorage), this storage uses a wrapper type for mutable /// accesses that only emits modification events when the component is explicitly /// accessed mutably which. This means that simply performing a mutable join will /// not, by itself, trigger a modification event. @@ -31,11 +32,14 @@ use shrev::EventChannel; /// To use this storage in a mutable join it should first be split into an [SplitFlaggedStorage] /// and an [EventPool] using [Storage::split]. This is because the deferred mutable /// access through the wrapper type requires passing in the channel separately. This strategy is -/// neccessary for soundness because [JoinIter] is not a streaming iterator so if we were to place +/// neccessary for soundness because [crate::join::JoinIter] is not a streaming iterator so if we were to place /// a mutable reference to the event channel inside of the mutable component wrapper this will /// cause aliasing of mutable references. In addition, this strategy enables us to safely perform /// parallel mutable joins with components in this storage. /// +/// If immediately triggering the modification event is desired, mutable access on a single +/// component can be obtained using [Storage::get_mut_tracked]. +/// /// Note: no effort is made to ensure a particular ordering of the modification /// events that occur withing the scope of a single `split`. pub struct UnsplitFlaggedStorage> { @@ -167,8 +171,8 @@ where { /// Temporarily divide into a [SplitFlaggedStorage] which borrows the underlying /// storage and an [EventPool] which borrows the underlying event channel and - /// is used to collect modification events. This allows deferred mutable access of components - /// in mutable. + /// is used to collect modification events. This allows deferred tracked mutable access of components + /// in mutable joins. pub fn split(&mut self) -> (SplitFlaggedStorage<'_, C, T>, EventPool<'_, C>) { let masked_storage = self.data.deref_mut(); let split_storage = SplitFlaggedStorage { @@ -192,7 +196,7 @@ where } impl> UnsplitFlaggedStorage { - /// Access the data associated with an `Index` for mutation. Internally emits + /// Access the data associated with an [Index] for mutation. Internally emits /// a component modification event if event emission is not disabled for this /// storage. /// @@ -213,7 +217,7 @@ where D: DerefMut>, I: UnprotectedStorage, { - /// Tries to mutate the data associated with an `Entity`. Internally emits + /// Tries to mutate the data associated with an [Entity]. Internally emits /// a component modification event if event emission is not disabled for this /// storage. Conveinence method for [UnsplitFlaggedStorage] which normally requires /// .... TODO: describe this when details crystallize.