From 3adb0e21371d16822e8a50bcbf4374c1776fb817 Mon Sep 17 00:00:00 2001 From: Joy <51241057+maniwani@users.noreply.github.com> Date: Sat, 30 Jul 2022 11:08:02 -0700 Subject: [PATCH 01/46] SemiSafeCell --- crates/bevy_ptr/src/lib.rs | 92 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 8f1f476a906e2..fe051e81e99c8 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -290,3 +290,95 @@ impl<'a, T> UnsafeCellDeref<'a, T> for &'a UnsafeCell { self.get().read() } } + +/// A type similar to [`UnsafeCell`] except it can be constructed from a shared or mutable reference. +/// +/// This type tries to act "borrow-like" like which means that: +/// - It must always reference a valid instance of `T`. +/// - The lifetime `'a` accurately represents how long it is valid. +/// +/// However, while this cell can produce a mutable reference (only if created from one), +/// it *can* be cloned, so the user is responsible for upholding Rust's aliasing rules. +/// - A `&T` reference may be released to safe code and may co-exist with other `&T` references, +/// but not with a `&mut T` reference. +/// - A `&mut T` reference may be released to safe code provided no other `&mut T` or `&T` exists. A `&mut T` must always be unique. +/// +#[doc(hidden)] +pub enum SemiSafeCell<'a, T> { + /// Cannot produce references. + Empty, + /// Can produce shared references. + Ref(&'a T), + /// Can produce one shared or mutable reference before converting to + /// [`Ref`](SemiSafeCell::Ref) or [`Empty`](SemiSafeCell::Empty), respectively. + Mut(&'a UnsafeCell), +} + +impl<'a, T> SemiSafeCell<'a, T> { + /// Constructs a cell from a shared reference. + pub fn from_ref(value: &'a T) -> Self { + Self::Ref(value) + } + + /// Constructs a cell from a mutable reference. + pub fn from_mut(value: &'a mut T) -> Self { + // SAFETY: `&mut` ensures unique access. + unsafe { Self::Mut(&*(value as *mut T as *const UnsafeCell)) } + } + + /// Returns a shared reference to the underlying data. + /// + /// Note: This converts a [`Mut`](SemiSafeCell::Mut) cell to a [`Ref`](SemiSafeCell::Ref) cell afterwards. + /// + /// # Safety + /// + /// Caller must ensure there are no active mutable references to the underlying data. + /// + /// # Panics + /// + /// Panics if the cell is empty. + pub unsafe fn take_ref(&mut self) -> &'a T { + match self { + Self::Empty => panic!("cell is empty"), + Self::Ref(borrow) => borrow, + Self::Mut(cell) => { + let borrow = &*cell.get(); + *self = SemiSafeCell::Ref(borrow); + borrow + } + } + } + + /// Returns mutable reference to the underlying data. + /// + /// Note: This converts a [`Mut`](SemiSafeCell::Mut) cell to an [`Empty`](SemiSafeCell::Empty) cell afterwards. + /// + /// # Safety + /// + /// Caller must ensure access to the underlying data is unique (no active references, mutable or not). + /// + /// # Panics + /// + /// Panics if the cell is empty or was constructed from a shared reference. + pub unsafe fn take_mut(&mut self) -> &'a mut T { + match self { + Self::Empty => panic!("cell is empty"), + Self::Ref(_) => panic!("cannot return mutable reference from Ref cell"), + Self::Mut(cell) => { + let borrow = &mut *cell.get(); + *self = SemiSafeCell::Empty; + borrow + } + } + } +} + +impl Copy for SemiSafeCell<'_, T> {} +impl Clone for SemiSafeCell<'_, T> { + fn clone(&self) -> Self { + *self + } +} + +// SAFETY: Executor guarantees that systems with conflicting access do not run at the same time. +unsafe impl Send for SemiSafeCell<'_, T> {} From e606f8adf62794e0e0aed603ac051b146f92cf0d Mon Sep 17 00:00:00 2001 From: Joy <51241057+maniwani@users.noreply.github.com> Date: Sat, 30 Jul 2022 11:19:31 -0700 Subject: [PATCH 02/46] Update access.rs --- crates/bevy_ecs/src/query/access.rs | 80 +++++++++++++++++++++++++---- 1 file changed, 71 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index d9580bd4eb507..a4c47f5b48cca 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -13,9 +13,12 @@ pub struct Access { reads_and_writes: FixedBitSet, /// The exclusively-accessed elements. writes: FixedBitSet, - /// Is `true` if this has access to all elements in the collection? + /// Is `true` if this has access to all elements in the collection. /// This field is a performance optimization for `&World` (also harder to mess up for soundness). reads_all: bool, + /// Is `true` if this has exclusive access to all elements in the collection. + /// This field is a performance optimization for `&mut World` (also harder to mess up for soundness). + writes_all: bool, marker: PhantomData, } @@ -23,6 +26,7 @@ impl Default for Access { fn default() -> Self { Self { reads_all: false, + writes_all: false, reads_and_writes: Default::default(), writes: Default::default(), marker: PhantomData, @@ -64,7 +68,11 @@ impl Access { /// Returns `true` if this can exclusively access the element given by `index`. pub fn has_write(&self, index: T) -> bool { - self.writes.contains(index.sparse_set_index()) + if self.writes_all { + true + } else { + self.writes.contains(index.sparse_set_index()) + } } /// Sets this as having access to all indexed elements (i.e. `&World`). @@ -77,9 +85,21 @@ impl Access { self.reads_all } + /// Sets this as having exclusive access to all indexed elements (i.e. `&mut World`). + pub fn write_all(&mut self) { + self.reads_all = true; + self.writes_all = true; + } + + /// Returns `true` if this has exclusive access to all indexed elements (i.e. `&mut World`). + pub fn has_write_all(&self) -> bool { + self.writes_all + } + /// Removes all accesses. pub fn clear(&mut self) { self.reads_all = false; + self.writes_all = false; self.reads_and_writes.clear(); self.writes.clear(); } @@ -87,6 +107,7 @@ impl Access { /// Adds all access from `other`. pub fn extend(&mut self, other: &Access) { self.reads_all = self.reads_all || other.reads_all; + self.writes_all = self.writes_all || other.writes_all; self.reads_and_writes.union_with(&other.reads_and_writes); self.writes.union_with(&other.writes); } @@ -96,6 +117,12 @@ impl Access { /// `Access` instances are incompatible if one can write /// an element that the other can read or write. pub fn is_compatible(&self, other: &Access) -> bool { + // All systems make a `&World` reference before running to update change detection info. + // Since exclusive systems produce a `&mut World`, we cannot let other systems run. + if self.writes_all || other.writes_all { + return false; + } + // Only systems that do not write data are compatible with systems that operate on `&World`. if self.reads_all { return other.writes.count_ones(..) == 0; @@ -112,15 +139,31 @@ impl Access { /// Returns a vector of elements that the access and `other` cannot access at the same time. pub fn get_conflicts(&self, other: &Access) -> Vec { let mut conflicts = FixedBitSet::default(); - if self.reads_all { - conflicts.extend(other.writes.ones()); + + if self.writes_all { + conflicts.extend(other.reads_and_writes.ones()); } - if other.reads_all { - conflicts.extend(self.writes.ones()); + if other.writes_all { + conflicts.extend(self.reads_and_writes.ones()); + } + + if !(self.writes_all || other.writes_all) { + match (self.reads_all, other.reads_all) { + (false, false) => { + conflicts.extend(self.writes.intersection(&other.reads_and_writes)); + conflicts.extend(self.reads_and_writes.intersection(&other.writes)); + } + (false, true) => { + conflicts.extend(self.writes.ones()); + } + (true, false) => { + conflicts.extend(other.writes.ones()); + } + (true, true) => (), + } } - conflicts.extend(self.writes.intersection(&other.reads_and_writes)); - conflicts.extend(self.reads_and_writes.intersection(&other.writes)); + conflicts .ones() .map(SparseSetIndex::get_sparse_set_index) @@ -266,6 +309,11 @@ impl FilteredAccess { pub fn read_all(&mut self) { self.access.read_all(); } + + /// Sets the underlying unfiltered access as having exclusive access to all indexed elements. + pub fn write_all(&mut self) { + self.access.write_all(); + } } /// A collection of [`FilteredAccess`] instances. @@ -306,6 +354,20 @@ impl FilteredAccessSet { true } + /// Returns `true` if this and `filtered_access` can be active at the same time. + pub fn is_compatible_single(&self, filtered_access: &FilteredAccess) -> bool { + if self.combined_access.is_compatible(filtered_access.access()) { + return true; + } + for filtered in &self.filtered_accesses { + if !filtered.is_compatible(filtered_access) { + return false; + } + } + + true + } + /// Returns a vector of elements that this set and `other` cannot access at the same time. pub fn get_conflicts(&self, other: &FilteredAccessSet) -> Vec { // if the unfiltered access is incompatible, must check each pair @@ -320,7 +382,7 @@ impl FilteredAccessSet { conflicts.into_iter().collect() } - /// Returns a vector of elements that this set and `other` cannot access at the same time. + /// Returns a vector of elements that this set and `filtered_access` cannot access at the same time. pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> Vec { // if the unfiltered access is incompatible, must check each pair let mut conflicts = HashSet::new(); From 0539c83196c7cfc6dcd2c57d2a71ad0092be12c7 Mon Sep 17 00:00:00 2001 From: Joy <51241057+maniwani@users.noreply.github.com> Date: Sat, 30 Jul 2022 11:56:53 -0700 Subject: [PATCH 03/46] Update system params --- crates/bevy_ecs/macros/src/lib.rs | 34 +- .../src/system/commands/parallel_scope.rs | 14 +- crates/bevy_ecs/src/system/system_param.rs | 316 +++++++++++++++--- 3 files changed, 307 insertions(+), 57 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 68023e315ddb0..618b6e94af93e 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -196,7 +196,8 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { // Conflicting params in ParamSet are not accessible at the same time // ParamSets are guaranteed to not conflict with other SystemParams unsafe { - <#param::Fetch as SystemParamFetch<'a, 'a>>::get_param(&mut self.param_states.#index, &self.system_meta, self.world, self.change_tick) + let mut world = self.world; + <#param::Fetch as SystemParamFetch<'a, 'a>>::get_param(&mut self.param_states.#index, &self.system_meta, &mut world, self.change_tick) } } }); @@ -219,6 +220,14 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { where #(#param_fetch: ReadOnlySystemParamFetch,)* { } + // SAFETY: ParamSet will inherit the exclusivity (and safety) of its params. + unsafe impl<#(#param_fetch: for<'w1, 's1> SystemParamFetch<'w1, 's1>,)*> MaybeExclusive for ParamSetState<(#(#param_fetch,)*)> + { + fn is_exclusive() -> bool { + [#(<#param_fetch as MaybeExclusive>::is_exclusive()),*].iter().any(|&b| b) + } + } + // SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts // with any prior access, a panic will occur. @@ -266,15 +275,23 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { - ParamSet { + let param_set = ParamSet { param_states: &mut state.0, system_meta: system_meta.clone(), - world, + world: world.clone(), change_tick, + }; + + if ::is_exclusive() { + world.take_mut(); + } else { + world.take_ref(); } + + param_set } } @@ -411,7 +428,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { unsafe fn get_param( state: &'s mut Self, system_meta: &#path::system::SystemMeta, - world: &'w #path::world::World, + world: &mut #path::system::SemiSafeCell<'w, #path::world::World>, change_tick: u32, ) -> Self::Item { #struct_name { @@ -423,6 +440,13 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { // Safety: The `ParamState` is `ReadOnlySystemParamFetch`, so this can only read from the `World` unsafe impl #path::system::ReadOnlySystemParamFetch for FetchState #where_clause {} + + // SAFETY: Delegates to the param field types. + unsafe impl #impl_generics #path::system::MaybeExclusive for FetchState <(#(<#field_types as #path::system::SystemParam>::Fetch,)*), #punctuated_generic_idents> #where_clause { + fn is_exclusive() -> bool { + [#(<<#field_types as #path::system::SystemParam>::Fetch as #path::system::MaybeExclusive>::is_exclusive(),)*].iter().any(|&b| b) + } + } }; }) } diff --git a/crates/bevy_ecs/src/system/commands/parallel_scope.rs b/crates/bevy_ecs/src/system/commands/parallel_scope.rs index 573afa70aae51..fe7d48c1556a2 100644 --- a/crates/bevy_ecs/src/system/commands/parallel_scope.rs +++ b/crates/bevy_ecs/src/system/commands/parallel_scope.rs @@ -5,7 +5,8 @@ use thread_local::ThreadLocal; use crate::{ entity::Entities, prelude::World, - system::{SystemParam, SystemParamFetch, SystemParamState}, + ptr::SemiSafeCell, + system::{MaybeExclusive, SystemParam, SystemParamFetch, SystemParamState}, }; use super::{CommandQueue, Commands}; @@ -58,16 +59,23 @@ impl<'w, 's> SystemParamFetch<'w, 's> for ParallelCommandsState { unsafe fn get_param( state: &'s mut Self, _: &crate::system::SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, _: u32, ) -> Self::Item { ParallelCommands { state, - entities: world.entities(), + entities: world.take_ref().entities(), } } } +// SAFETY: ParallelCommandsState::get_param constructs a &World. +unsafe impl MaybeExclusive for ParallelCommandsState { + fn is_exclusive() -> bool { + false + } +} + // SAFETY: no component or resource access to report unsafe impl SystemParamState for ParallelCommandsState { fn init(_: &mut World, _: &mut crate::system::SystemMeta) -> Self { diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 0472e45c367c6..7442d0ee5d919 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -5,14 +5,13 @@ use crate::{ change_detection::Ticks, component::{Component, ComponentId, ComponentTicks, Components}, entity::{Entities, Entity}, - query::{ - Access, FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyWorldQuery, WorldQuery, - }, + query::{FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyWorldQuery, WorldQuery}, system::{CommandQueue, Commands, Query, SystemMeta}, world::{FromWorld, World}, }; pub use bevy_ecs_macros::SystemParam; use bevy_ecs_macros::{all_tuples, impl_param_set}; +pub use bevy_ptr::SemiSafeCell; use bevy_ptr::UnsafeCellDeref; use std::{ fmt::Debug, @@ -116,7 +115,7 @@ pub unsafe trait SystemParamState: Send + Sync + 'static { /// This must only be implemented for [`SystemParamFetch`] impls that exclusively read the World passed in to [`SystemParamFetch::get_param`] pub unsafe trait ReadOnlySystemParamFetch {} -pub trait SystemParamFetch<'world, 'state>: SystemParamState { +pub trait SystemParamFetch<'world, 'state>: SystemParamState + MaybeExclusive { type Item: SystemParam; /// # Safety /// @@ -125,11 +124,22 @@ pub trait SystemParamFetch<'world, 'state>: SystemParamState { unsafe fn get_param( state: &'state mut Self, system_meta: &SystemMeta, - world: &'world World, + world: &mut SemiSafeCell<'world, World>, change_tick: u32, ) -> Self::Item; } +/// A [`SystemParamFetch`] type that knows if it mutably references the [`World`]. +/// +/// # Safety +/// +/// - [`is_exclusive`](MaybeExclusive::is_exclusive) must return `true` +/// if the type's [`get_param`](SystemParamFetch::get_param) constructs a [`&mut World`](World) +/// or a type that can do so. +pub unsafe trait MaybeExclusive { + fn is_exclusive() -> bool; +} + impl<'w, 's, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParam for Query<'w, 's, Q, F> { type Fetch = QueryState; } @@ -178,10 +188,22 @@ impl<'w, 's, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParamFetch< unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { - Query::new(world, state, system_meta.last_change_tick, change_tick) + Query::new( + world.take_ref(), + state, + system_meta.last_change_tick, + change_tick, + ) + } +} + +// SAFETY: QueryState::get_param constructs a &World. +unsafe impl MaybeExclusive for QueryState { + fn is_exclusive() -> bool { + false } } @@ -208,7 +230,7 @@ fn assert_component_access_compatibility( pub struct ParamSet<'w, 's, T: SystemParam> { param_states: &'s mut T::Fetch, - world: &'w World, + world: SemiSafeCell<'w, World>, system_meta: SystemMeta, change_tick: u32, } @@ -340,10 +362,11 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for ResState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { let column = world + .take_ref() .get_populated_resource_column(state.component_id) .unwrap_or_else(|| { panic!( @@ -361,6 +384,13 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for ResState { } } +// SAFETY: ResState::get_param constructs a &World. +unsafe impl MaybeExclusive for ResState { + fn is_exclusive() -> bool { + false + } +} + /// The [`SystemParamState`] of [`Option>`]. /// See: [`Res`] #[doc(hidden)] @@ -388,10 +418,11 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for OptionResState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { world + .take_ref() .get_populated_resource_column(state.0.component_id) .map(|column| Res { value: column.get_data_ptr().deref::(), @@ -402,6 +433,13 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for OptionResState { } } +// SAFETY: Option>::get_param constructs a &World. +unsafe impl MaybeExclusive for OptionResState { + fn is_exclusive() -> bool { + false + } +} + /// The [`SystemParamState`] of [`ResMut`]. #[doc(hidden)] pub struct ResMutState { @@ -451,10 +489,11 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for ResMutState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { let value = world + .take_ref() .get_resource_unchecked_mut_with_id(state.component_id) .unwrap_or_else(|| { panic!( @@ -474,6 +513,13 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for ResMutState { } } +// SAFETY: ResMutState::get_param constructs a &World. +unsafe impl MaybeExclusive for ResMutState { + fn is_exclusive() -> bool { + false + } +} + /// The [`SystemParamState`] of [`Option>`]. /// See: [`ResMut`] #[doc(hidden)] @@ -498,10 +544,11 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for OptionResMutState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { world + .take_ref() .get_resource_unchecked_mut_with_id(state.0.component_id) .map(|value| ResMut { value: value.value, @@ -514,6 +561,13 @@ impl<'w, 's, T: Resource> SystemParamFetch<'w, 's> for OptionResMutState { } } +// SAFETY: OptionResMutState::get_param constructs a &World. +unsafe impl MaybeExclusive for OptionResMutState { + fn is_exclusive() -> bool { + false + } +} + impl<'w, 's> SystemParam for Commands<'w, 's> { type Fetch = CommandQueue; } @@ -539,10 +593,17 @@ impl<'w, 's> SystemParamFetch<'w, 's> for CommandQueue { unsafe fn get_param( state: &'s mut Self, _system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, _change_tick: u32, ) -> Self::Item { - Commands::new(state, world) + Commands::new(state, world.take_ref()) + } +} + +// SAFETY: CommandQueue::get_param constructs a &World. +unsafe impl MaybeExclusive for CommandQueue { + fn is_exclusive() -> bool { + false } } @@ -560,41 +621,104 @@ impl<'w> SystemParam for &'w World { // SAFETY: `read_all` access is set and conflicts result in a panic unsafe impl SystemParamState for WorldState { fn init(_world: &mut World, system_meta: &mut SystemMeta) -> Self { - let mut access = Access::default(); - access.read_all(); + let mut world_access = FilteredAccess::default(); + world_access.read_all(); + + // conflict with any preceding param if !system_meta - .archetype_component_access - .is_compatible(&access) + .component_access_set + .is_compatible_single(&world_access) { - panic!("&World conflicts with a previous mutable system parameter. Allowing this would break Rust's mutability rules"); + panic!( + "&World conflicts with another system param in {}. \ + Mutable access must be unique.", + system_meta.name, + ); } - system_meta.archetype_component_access.extend(&access); - let mut filtered_access = FilteredAccess::default(); + // conflict with any following param + system_meta.component_access_set.add(world_access); + + // executor + system_meta.archetype_component_access.read_all(); + + WorldState + } +} + +impl<'w, 's> SystemParamFetch<'w, 's> for WorldState { + type Item = &'w World; + unsafe fn get_param( + _state: &'s mut Self, + _system_meta: &SystemMeta, + world: &mut SemiSafeCell<'w, World>, + _change_tick: u32, + ) -> Self::Item { + world.take_ref() + } +} + +// SAFETY: WorldState::get_param constructs a &World. +unsafe impl MaybeExclusive for WorldState { + fn is_exclusive() -> bool { + false + } +} + +/// The [`SystemParamState`] of [`&mut World`](crate::world::World). +#[doc(hidden)] +pub struct WorldMutState; + +impl<'w> SystemParam for &'w mut World { + type Fetch = WorldMutState; +} + +unsafe impl SystemParamState for WorldMutState { + fn init(_world: &mut World, system_meta: &mut SystemMeta) -> Self { + let mut world_access = FilteredAccess::default(); + world_access.write_all(); - filtered_access.read_all(); + // conflict with any preceding param if !system_meta .component_access_set - .get_conflicts_single(&filtered_access) - .is_empty() + .is_compatible_single(&world_access) { - panic!("&World conflicts with a previous mutable system parameter. Allowing this would break Rust's mutability rules"); + panic!( + "&mut World conflicts with another system param in {}. \ + Mutable access must be unique.", + system_meta.name, + ); } - system_meta.component_access_set.add(filtered_access); - WorldState + // conflict with any following param + system_meta.component_access_set.add(world_access); + + // executor + system_meta.archetype_component_access.write_all(); + + // world can contain non-send resources, must run on its local thread + system_meta.set_non_send(); + + WorldMutState } } -impl<'w, 's> SystemParamFetch<'w, 's> for WorldState { - type Item = &'w World; +impl<'w, 's> SystemParamFetch<'w, 's> for WorldMutState { + type Item = &'w mut World; unsafe fn get_param( _state: &'s mut Self, _system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, _change_tick: u32, ) -> Self::Item { - world + world.take_mut() + } +} + +// SAFETY: WorldMutState::get_param does construct a `&mut World`. +unsafe impl MaybeExclusive for WorldMutState { + fn is_exclusive() -> bool { + true } } @@ -692,13 +816,20 @@ impl<'w, 's, T: Resource + FromWorld> SystemParamFetch<'w, 's> for LocalState unsafe fn get_param( state: &'s mut Self, _system_meta: &SystemMeta, - _world: &'w World, + _world: &mut SemiSafeCell<'w, World>, _change_tick: u32, ) -> Self::Item { Local(&mut state.0) } } +// SAFETY: LocalState::get_param does not construct any `World` pointer or reference. +unsafe impl MaybeExclusive for LocalState { + fn is_exclusive() -> bool { + false + } +} + /// A [`SystemParam`] that grants access to the entities that had their `T` [`Component`] removed. /// /// Note that this does not allow you to see which data existed before removal. @@ -778,17 +909,24 @@ impl<'w, 's, T: Component> SystemParamFetch<'w, 's> for RemovedComponentsState, _change_tick: u32, ) -> Self::Item { RemovedComponents { - world, + world: world.take_ref(), component_id: state.component_id, marker: PhantomData, } } } +// SAFETY: RemovedComponentsState::get_param constructs a &World. +unsafe impl MaybeExclusive for RemovedComponentsState { + fn is_exclusive() -> bool { + false + } +} + /// Shared borrow of a non-[`Send`] resource. /// /// Only `Send` resources may be accessed with the [`Res`] [`SystemParam`]. In case that the @@ -899,9 +1037,10 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { + let world = world.take_ref(); world.validate_non_send_access::(); let column = world .get_populated_resource_column(state.component_id) @@ -922,6 +1061,13 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendState { } } +// SAFETY: NonSendState::get_param constructs a &World. +unsafe impl MaybeExclusive for NonSendState { + fn is_exclusive() -> bool { + false + } +} + /// The [`SystemParamState`] of [`Option>`]. /// See: [`NonSend`] #[doc(hidden)] @@ -949,9 +1095,10 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { + let world = world.take_ref(); world.validate_non_send_access::(); world .get_populated_resource_column(state.0.component_id) @@ -964,6 +1111,13 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendState { } } +// SAFETY: OptionNonSendState::get_param constructs a &World. +unsafe impl MaybeExclusive for OptionNonSendState { + fn is_exclusive() -> bool { + false + } +} + /// The [`SystemParamState`] of [`NonSendMut`]. #[doc(hidden)] pub struct NonSendMutState { @@ -1015,9 +1169,10 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendMutState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { + let world = world.take_ref(); world.validate_non_send_access::(); let column = world .get_populated_resource_column(state.component_id) @@ -1039,6 +1194,13 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendMutState { } } +// SAFETY: NonSendMutState::get_param constructs a &World. +unsafe impl MaybeExclusive for NonSendMutState { + fn is_exclusive() -> bool { + false + } +} + /// The [`SystemParamState`] of [`Option>`]. /// See: [`NonSendMut`] #[doc(hidden)] @@ -1063,9 +1225,10 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendMutState { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { + let world = world.take_ref(); world.validate_non_send_access::(); world .get_populated_resource_column(state.0.component_id) @@ -1080,6 +1243,13 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendMutState { } } +// SAFETY: OptionNonSendMutState::get_param constructs a &World. +unsafe impl MaybeExclusive for OptionNonSendMutState { + fn is_exclusive() -> bool { + false + } +} + impl<'a> SystemParam for &'a Archetypes { type Fetch = ArchetypesState; } @@ -1105,10 +1275,17 @@ impl<'w, 's> SystemParamFetch<'w, 's> for ArchetypesState { unsafe fn get_param( _state: &'s mut Self, _system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, _change_tick: u32, ) -> Self::Item { - world.archetypes() + world.take_ref().archetypes() + } +} + +// SAFETY: ArchetypesState::get_param constructs a &World. +unsafe impl MaybeExclusive for ArchetypesState { + fn is_exclusive() -> bool { + false } } @@ -1137,10 +1314,17 @@ impl<'w, 's> SystemParamFetch<'w, 's> for ComponentsState { unsafe fn get_param( _state: &'s mut Self, _system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, _change_tick: u32, ) -> Self::Item { - world.components() + world.take_ref().components() + } +} + +// SAFETY: ComponentsState::get_param constructs a &World. +unsafe impl MaybeExclusive for ComponentsState { + fn is_exclusive() -> bool { + false } } @@ -1169,10 +1353,17 @@ impl<'w, 's> SystemParamFetch<'w, 's> for EntitiesState { unsafe fn get_param( _state: &'s mut Self, _system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, _change_tick: u32, ) -> Self::Item { - world.entities() + world.take_ref().entities() + } +} + +// SAFETY: EntitiesState::get_param constructs a &World. +unsafe impl MaybeExclusive for EntitiesState { + fn is_exclusive() -> bool { + false } } @@ -1201,10 +1392,17 @@ impl<'w, 's> SystemParamFetch<'w, 's> for BundlesState { unsafe fn get_param( _state: &'s mut Self, _system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, _change_tick: u32, ) -> Self::Item { - world.bundles() + world.take_ref().bundles() + } +} + +// SAFETY: BundlesState::get_param constructs a &World. +unsafe impl MaybeExclusive for BundlesState { + fn is_exclusive() -> bool { + false } } @@ -1261,7 +1459,7 @@ impl<'w, 's> SystemParamFetch<'w, 's> for SystemChangeTickState { unsafe fn get_param( _state: &'s mut Self, system_meta: &SystemMeta, - _world: &'w World, + _world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { SystemChangeTick { @@ -1271,6 +1469,13 @@ impl<'w, 's> SystemParamFetch<'w, 's> for SystemChangeTickState { } } +// SAFETY: SystemChangeTickState::get_param does not construct a World pointer or reference. +unsafe impl MaybeExclusive for SystemChangeTickState { + fn is_exclusive() -> bool { + false + } +} + macro_rules! impl_system_param_tuple { ($($param: ident),*) => { impl<$($param: SystemParam),*> SystemParam for ($($param,)*) { @@ -1280,6 +1485,12 @@ macro_rules! impl_system_param_tuple { // SAFETY: tuple consists only of ReadOnlySystemParamFetches unsafe impl<$($param: ReadOnlySystemParamFetch),*> ReadOnlySystemParamFetch for ($($param,)*) {} + unsafe impl<$($param: MaybeExclusive),*> MaybeExclusive for ($($param,)*) { + fn is_exclusive() -> bool { + [$($param::is_exclusive()),*].iter().any(|&b| b) + } + } + #[allow(unused_variables)] #[allow(non_snake_case)] impl<'w, 's, $($param: SystemParamFetch<'w, 's>),*> SystemParamFetch<'w, 's> for ($($param,)*) { @@ -1290,7 +1501,7 @@ macro_rules! impl_system_param_tuple { unsafe fn get_param( state: &'s mut Self, system_meta: &SystemMeta, - world: &'w World, + world: &mut SemiSafeCell<'w, World>, change_tick: u32, ) -> Self::Item { @@ -1420,6 +1631,13 @@ unsafe impl ReadOnlySystemParamFetch { } +// SAFETY: Delegates to S +unsafe impl MaybeExclusive for StaticSystemParamState { + fn is_exclusive() -> bool { + S::is_exclusive() + } +} + impl<'world, 'state, P: SystemParam + 'static> SystemParam for StaticSystemParam<'world, 'state, P> { @@ -1436,7 +1654,7 @@ where unsafe fn get_param( state: &'state mut Self, system_meta: &SystemMeta, - world: &'world World, + world: &mut SemiSafeCell<'world, World>, change_tick: u32, ) -> Self::Item { // SAFETY: We properly delegate SystemParamState From b47d610add3d7e0f0265c18b46404815839cd824 Mon Sep 17 00:00:00 2001 From: Joy <51241057+maniwani@users.noreply.github.com> Date: Sat, 30 Jul 2022 14:13:24 -0700 Subject: [PATCH 04/46] Update system --- crates/bevy_ecs/src/system/function_system.rs | 80 +++++++++++++------ crates/bevy_ecs/src/system/system.rs | 8 +- crates/bevy_ecs/src/system/system_chaining.rs | 7 +- 3 files changed, 68 insertions(+), 27 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index a1e209e2426d8..173c73cd2b6fe 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -3,11 +3,12 @@ use crate::{ change_detection::MAX_CHANGE_AGE, component::ComponentId, prelude::FromWorld, + ptr::SemiSafeCell, query::{Access, FilteredAccessSet}, schedule::{SystemLabel, SystemLabelId}, system::{ - check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch, - SystemParamItem, SystemParamState, + check_system_change_tick, MaybeExclusive, ReadOnlySystemParamFetch, System, SystemParam, + SystemParamFetch, SystemParamItem, SystemParamState, }, world::{World, WorldId}, }; @@ -167,7 +168,7 @@ impl SystemState { { self.validate_world_and_update_archetypes(world); // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. - unsafe { self.get_unchecked_manual(world) } + unsafe { self.get_unchecked_manual(SemiSafeCell::from_ref(world)) } } /// Retrieve the mutable [`SystemParam`] values. @@ -178,7 +179,7 @@ impl SystemState { ) -> >::Item { self.validate_world_and_update_archetypes(world); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. - unsafe { self.get_unchecked_manual(world) } + unsafe { self.get_unchecked_manual(SemiSafeCell::from_mut(world)) } } /// Applies all state queued up for [`SystemParam`] values. For example, this will apply commands queued up @@ -218,13 +219,14 @@ impl SystemState { #[inline] pub unsafe fn get_unchecked_manual<'w, 's>( &'s mut self, - world: &'w World, + mut world: SemiSafeCell<'w, World>, ) -> >::Item { - let change_tick = world.increment_change_tick(); + let mut cell = world; + let change_tick = cell.take_ref().increment_change_tick(); let param = ::get_param( &mut self.param_state, &self.meta, - world, + &mut world, change_tick, ); self.meta.last_change_tick = change_tick; @@ -387,22 +389,54 @@ where } #[inline] - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { - let change_tick = world.increment_change_tick(); - - // Safety: - // We update the archetype component access correctly based on `Param`'s requirements - // in `update_archetype_component_access`. - // Our caller upholds the requirements. - let params = ::Fetch::get_param( - self.param_state.as_mut().expect(Self::PARAM_MESSAGE), - &self.system_meta, - world, - change_tick, - ); - let out = self.func.run(input, params); - self.system_meta.last_change_tick = change_tick; - out + fn is_exclusive(&self) -> bool { + ::is_exclusive() + } + + #[inline] + unsafe fn run_unsafe(&mut self, input: Self::In, mut world: SemiSafeCell) -> Self::Out { + if self.is_exclusive() { + // exclusive systems temporarily swap out the world's previous change tick + // so that smart pointers directly created from the world work as expected + let mut cell = world; + let world_mut = cell.clone().take_mut(); + let original_last_change_tick = world_mut.last_change_tick; + world_mut.last_change_tick = self.system_meta.last_change_tick; + let change_tick = *world_mut.change_tick.get_mut(); + + let params = ::Fetch::get_param( + self.param_state.as_mut().expect(Self::PARAM_MESSAGE), + &self.system_meta, + &mut world, + change_tick, + ); + let out = self.func.run(input, params); + + let world_mut = cell.take_mut(); + let change_tick = world_mut.change_tick.get_mut(); + self.system_meta.last_change_tick = *change_tick; + *change_tick += 1; + world_mut.last_change_tick = original_last_change_tick; + out + } else { + let mut cell = world; + let world_ref = cell.take_ref(); + let change_tick = world_ref.increment_change_tick(); + + // Safety: + // We update the archetype component access correctly based on `Param`'s requirements + // in `update_archetype_component_access`. + // Our caller upholds the requirements. + let params = ::Fetch::get_param( + self.param_state.as_mut().expect(Self::PARAM_MESSAGE), + &self.system_meta, + &mut world, + change_tick, + ); + let out = self.func.run(input, params); + self.system_meta.last_change_tick = change_tick; + out + } } #[inline] diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 6a7139b25b35d..5754323ea9fdc 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -2,7 +2,7 @@ use bevy_utils::tracing::warn; use crate::{ archetype::ArchetypeComponentId, change_detection::MAX_CHANGE_AGE, component::ComponentId, - query::Access, schedule::SystemLabelId, world::World, + ptr::SemiSafeCell, query::Access, schedule::SystemLabelId, world::World, }; use std::borrow::Cow; @@ -31,6 +31,8 @@ pub trait System: Send + Sync + 'static { fn archetype_component_access(&self) -> &Access; /// Returns true if the system is [`Send`]. fn is_send(&self) -> bool; + /// Returns `true` if the system cannot run in parallel with other systems. + fn is_exclusive(&self) -> bool; /// Runs the system with the given input in the world. Unlike [`System::run`], this function /// takes a shared reference to [`World`] and may therefore break Rust's aliasing rules, making /// it unsafe to call. @@ -42,12 +44,12 @@ pub trait System: Send + Sync + 'static { /// 1. This system is the only system running on the given world across all threads. /// 2. This system only runs in parallel with other systems that do not conflict with the /// [`System::archetype_component_access()`]. - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out; + unsafe fn run_unsafe(&mut self, input: Self::In, world: SemiSafeCell) -> Self::Out; /// Runs the system with the given input in the world. fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { self.update_archetype_component_access(world); // SAFETY: world and resources are exclusively borrowed - unsafe { self.run_unsafe(input, world) } + unsafe { self.run_unsafe(input, SemiSafeCell::from_mut(world)) } } fn apply_buffers(&mut self, world: &mut World); /// Initialize the system. diff --git a/crates/bevy_ecs/src/system/system_chaining.rs b/crates/bevy_ecs/src/system/system_chaining.rs index cf8e27b5ad757..debea216c3ecf 100644 --- a/crates/bevy_ecs/src/system/system_chaining.rs +++ b/crates/bevy_ecs/src/system/system_chaining.rs @@ -1,6 +1,7 @@ use crate::{ archetype::ArchetypeComponentId, component::ComponentId, + ptr::SemiSafeCell, query::Access, system::{IntoSystem, System}, world::World, @@ -72,7 +73,11 @@ impl> System for ChainSystem self.system_a.is_send() && self.system_b.is_send() } - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { + fn is_exclusive(&self) -> bool { + self.system_a.is_exclusive() || self.system_b.is_exclusive() + } + + unsafe fn run_unsafe(&mut self, input: Self::In, world: SemiSafeCell) -> Self::Out { let out = self.system_a.run_unsafe(input, world); self.system_b.run_unsafe(out, world) } From 8e953175e7a2e5c682e13f9411893663f27445b2 Mon Sep 17 00:00:00 2001 From: Joy <51241057+maniwani@users.noreply.github.com> Date: Sat, 30 Jul 2022 14:12:15 -0700 Subject: [PATCH 05/46] remove ExclusiveSystem* internals For future reference if I have to solve the ambiguity_detection test a third time, the else branch in find_ambiguities is no longer taken because exclusive systems have a component access now. And obv two empty systems will always be compatible. --- crates/bevy_ecs/src/entity/mod.rs | 2 +- crates/bevy_ecs/src/lib.rs | 10 +- crates/bevy_ecs/src/schedule/executor.rs | 10 +- .../src/schedule/executor_parallel.rs | 15 +- crates/bevy_ecs/src/schedule/stage.rs | 459 ++++++++++-------- .../bevy_ecs/src/schedule/system_container.rs | 98 +--- .../src/schedule/system_descriptor.rs | 376 ++++++-------- crates/bevy_ecs/src/schedule/system_set.rs | 24 +- .../bevy_ecs/src/system/exclusive_system.rs | 171 ------- crates/bevy_ecs/src/system/mod.rs | 36 +- 10 files changed, 443 insertions(+), 758 deletions(-) delete mode 100644 crates/bevy_ecs/src/system/exclusive_system.rs diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index e022483fa836c..da2d6266363b7 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -72,7 +72,7 @@ use std::{ /// } /// # /// # bevy_ecs::system::assert_is_system(setup); -/// # bevy_ecs::system::IntoExclusiveSystem::exclusive_system(exclusive_system); +/// # bevy_ecs::system::assert_is_system(exclusive_system); /// ``` /// /// It can be used to refer to a specific entity to apply [`EntityCommands`], or to call [`Query::get`] (or similar methods) to access its components. diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index cafacc2a2d929..6077b6e068700 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -34,13 +34,13 @@ pub mod prelude { event::{EventReader, EventWriter, Events}, query::{Added, AnyOf, ChangeTrackers, Changed, Or, QueryState, With, Without}, schedule::{ - AmbiguitySetLabel, ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, - RunCriteria, RunCriteriaDescriptorCoercion, RunCriteriaLabel, Schedule, Stage, - StageLabel, State, SystemLabel, SystemSet, SystemStage, + AmbiguitySetLabel, IntoSystemDescriptor, RunCriteria, RunCriteriaDescriptorCoercion, + RunCriteriaLabel, Schedule, Stage, StageLabel, State, SystemLabel, SystemSet, + SystemStage, }, system::{ - Commands, In, IntoChainSystem, IntoExclusiveSystem, IntoSystem, Local, NonSend, - NonSendMut, ParallelCommands, ParamSet, Query, RemovedComponents, Res, ResMut, System, + Commands, In, IntoChainSystem, IntoSystem, Local, NonSend, NonSendMut, + ParallelCommands, ParamSet, Query, RemovedComponents, Res, ResMut, System, SystemParamFunction, }, world::{FromWorld, Mut, World}, diff --git a/crates/bevy_ecs/src/schedule/executor.rs b/crates/bevy_ecs/src/schedule/executor.rs index 06db889ec2d6c..edf6fcf5c6ab3 100644 --- a/crates/bevy_ecs/src/schedule/executor.rs +++ b/crates/bevy_ecs/src/schedule/executor.rs @@ -1,11 +1,11 @@ -use crate::{schedule::ParallelSystemContainer, world::World}; +use crate::{schedule::FunctionSystemContainer, world::World}; use downcast_rs::{impl_downcast, Downcast}; pub trait ParallelSystemExecutor: Downcast + Send + Sync { /// Called by `SystemStage` whenever `systems` have been changed. - fn rebuild_cached_data(&mut self, systems: &[ParallelSystemContainer]); + fn rebuild_cached_data(&mut self, systems: &[FunctionSystemContainer]); - fn run_systems(&mut self, systems: &mut [ParallelSystemContainer], world: &mut World); + fn run_systems(&mut self, systems: &mut [FunctionSystemContainer], world: &mut World); } impl_downcast!(ParallelSystemExecutor); @@ -14,9 +14,9 @@ impl_downcast!(ParallelSystemExecutor); pub struct SingleThreadedExecutor; impl ParallelSystemExecutor for SingleThreadedExecutor { - fn rebuild_cached_data(&mut self, _: &[ParallelSystemContainer]) {} + fn rebuild_cached_data(&mut self, _: &[FunctionSystemContainer]) {} - fn run_systems(&mut self, systems: &mut [ParallelSystemContainer], world: &mut World) { + fn run_systems(&mut self, systems: &mut [FunctionSystemContainer], world: &mut World) { for system in systems { if system.should_run() { #[cfg(feature = "trace")] diff --git a/crates/bevy_ecs/src/schedule/executor_parallel.rs b/crates/bevy_ecs/src/schedule/executor_parallel.rs index 4bf36da141de0..cf4e9307fa99c 100644 --- a/crates/bevy_ecs/src/schedule/executor_parallel.rs +++ b/crates/bevy_ecs/src/schedule/executor_parallel.rs @@ -1,10 +1,11 @@ use crate::{ archetype::ArchetypeComponentId, query::Access, - schedule::{ParallelSystemContainer, ParallelSystemExecutor}, + schedule::{FunctionSystemContainer, ParallelSystemExecutor}, world::World, }; use async_channel::{Receiver, Sender}; +use bevy_ptr::SemiSafeCell; use bevy_tasks::{ComputeTaskPool, Scope, TaskPool}; #[cfg(feature = "trace")] use bevy_utils::tracing::Instrument; @@ -74,7 +75,7 @@ impl Default for ParallelExecutor { } impl ParallelSystemExecutor for ParallelExecutor { - fn rebuild_cached_data(&mut self, systems: &[ParallelSystemContainer]) { + fn rebuild_cached_data(&mut self, systems: &[FunctionSystemContainer]) { self.system_metadata.clear(); self.queued.grow(systems.len()); self.running.grow(systems.len()); @@ -82,6 +83,10 @@ impl ParallelSystemExecutor for ParallelExecutor { // Construct scheduling data for systems. for container in systems { + if container.system().is_exclusive() { + panic!("executor cannot run exclusive systems"); + } + let dependencies_total = container.dependencies().len(); let system = container.system(); let (start_sender, start_receiver) = async_channel::bounded(1); @@ -103,7 +108,7 @@ impl ParallelSystemExecutor for ParallelExecutor { } } - fn run_systems(&mut self, systems: &mut [ParallelSystemContainer], world: &mut World) { + fn run_systems(&mut self, systems: &mut [FunctionSystemContainer], world: &mut World) { #[cfg(test)] if self.events_sender.is_none() { let (sender, receiver) = async_channel::unbounded::(); @@ -163,7 +168,7 @@ impl ParallelExecutor { fn prepare_systems<'scope>( &mut self, scope: &mut Scope<'scope, ()>, - systems: &'scope mut [ParallelSystemContainer], + systems: &'scope mut [FunctionSystemContainer], world: &'scope World, ) { #[cfg(feature = "trace")] @@ -191,7 +196,7 @@ impl ParallelExecutor { #[cfg(feature = "trace")] let system_guard = system_span.enter(); // SAFETY: the executor prevents two systems with conflicting access from running simultaneously. - unsafe { system.run_unsafe((), world) }; + unsafe { system.run_unsafe((), SemiSafeCell::from_ref(world)) }; #[cfg(feature = "trace")] drop(system_guard); finish_sender diff --git a/crates/bevy_ecs/src/schedule/stage.rs b/crates/bevy_ecs/src/schedule/stage.rs index a14a5db7be32a..68e89fba41293 100644 --- a/crates/bevy_ecs/src/schedule/stage.rs +++ b/crates/bevy_ecs/src/schedule/stage.rs @@ -4,11 +4,11 @@ use crate::{ prelude::IntoSystem, schedule::{ graph_utils::{self, DependencyGraphError}, - BoxedRunCriteria, DuplicateLabelStrategy, ExclusiveSystemContainer, GraphNode, - InsertionPoint, ParallelExecutor, ParallelSystemContainer, ParallelSystemExecutor, - RunCriteriaContainer, RunCriteriaDescriptor, RunCriteriaDescriptorOrLabel, - RunCriteriaInner, RunCriteriaLabelId, ShouldRun, SingleThreadedExecutor, SystemContainer, - SystemDescriptor, SystemLabelId, SystemSet, + BoxedRunCriteria, DuplicateLabelStrategy, FunctionSystemContainer, GraphNode, + InsertionPoint, ParallelExecutor, ParallelSystemExecutor, RunCriteriaContainer, + RunCriteriaDescriptor, RunCriteriaDescriptorOrLabel, RunCriteriaInner, RunCriteriaLabelId, + ShouldRun, SingleThreadedExecutor, SystemContainer, SystemDescriptor, SystemLabelId, + SystemSet, SystemType, }, world::{World, WorldId}, }; @@ -65,14 +65,14 @@ pub struct SystemStage { /// Topologically sorted run criteria of systems. run_criteria: Vec, /// Topologically sorted exclusive systems that want to be run at the start of the stage. - exclusive_at_start: Vec, + exclusive_at_start: Vec, /// Topologically sorted exclusive systems that want to be run after parallel systems but /// before the application of their command buffers. - exclusive_before_commands: Vec, + exclusive_before_commands: Vec, /// Topologically sorted exclusive systems that want to be run at the end of the stage. - exclusive_at_end: Vec, + exclusive_at_end: Vec, /// Topologically sorted parallel systems. - parallel: Vec, + parallel: Vec, /// Determines if the stage was modified and needs to rebuild its graphs and orders. systems_modified: bool, /// Determines if the stage's executor was changed. @@ -80,13 +80,7 @@ pub struct SystemStage { /// Newly inserted run criteria that will be initialized at the next opportunity. uninitialized_run_criteria: Vec<(usize, DuplicateLabelStrategy)>, /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_at_start: Vec, - /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_before_commands: Vec, - /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_at_end: Vec, - /// Newly inserted systems that will be initialized at the next opportunity. - uninitialized_parallel: Vec, + uninitialized_systems: Vec<(FunctionSystemContainer, SystemType)>, /// Saves the value of the World change_tick during the last tick check last_tick_check: u32, /// If true, buffers will be automatically applied at the end of the stage. If false, buffers must be manually applied. @@ -102,16 +96,13 @@ impl SystemStage { stage_run_criteria: Default::default(), run_criteria: vec![], uninitialized_run_criteria: vec![], + uninitialized_systems: vec![], exclusive_at_start: Default::default(), exclusive_before_commands: Default::default(), exclusive_at_end: Default::default(), parallel: vec![], systems_modified: true, executor_modified: true, - uninitialized_parallel: vec![], - uninitialized_at_start: vec![], - uninitialized_before_commands: vec![], - uninitialized_at_end: vec![], last_tick_check: Default::default(), apply_buffers: true, must_read_resource: None, @@ -159,64 +150,29 @@ impl SystemStage { self } - fn add_system_inner(&mut self, system: SystemDescriptor, default_run_criteria: Option) { + fn add_system_inner( + &mut self, + mut descriptor: SystemDescriptor, + default_run_criteria: Option, + ) { self.systems_modified = true; - match system { - SystemDescriptor::Exclusive(mut descriptor) => { - let insertion_point = descriptor.insertion_point; - let criteria = descriptor.run_criteria.take(); - let mut container = ExclusiveSystemContainer::from_descriptor(descriptor); - match criteria { - Some(RunCriteriaDescriptorOrLabel::Label(label)) => { - container.run_criteria_label = Some(label); - } - Some(RunCriteriaDescriptorOrLabel::Descriptor(criteria_descriptor)) => { - container.run_criteria_label = criteria_descriptor.label; - container.run_criteria_index = - Some(self.add_run_criteria_internal(criteria_descriptor)); - } - None => { - container.run_criteria_index = default_run_criteria; - } - } - match insertion_point { - InsertionPoint::AtStart => { - let index = self.exclusive_at_start.len(); - self.uninitialized_at_start.push(index); - self.exclusive_at_start.push(container); - } - InsertionPoint::BeforeCommands => { - let index = self.exclusive_before_commands.len(); - self.uninitialized_before_commands.push(index); - self.exclusive_before_commands.push(container); - } - InsertionPoint::AtEnd => { - let index = self.exclusive_at_end.len(); - self.uninitialized_at_end.push(index); - self.exclusive_at_end.push(container); - } - } + let system_type = descriptor.system_type; + let criteria = descriptor.run_criteria.take(); + let mut container = FunctionSystemContainer::from_descriptor(descriptor); + match criteria { + Some(RunCriteriaDescriptorOrLabel::Label(label)) => { + container.run_criteria_label = Some(label); } - SystemDescriptor::Parallel(mut descriptor) => { - let criteria = descriptor.run_criteria.take(); - let mut container = ParallelSystemContainer::from_descriptor(descriptor); - match criteria { - Some(RunCriteriaDescriptorOrLabel::Label(label)) => { - container.run_criteria_label = Some(label); - } - Some(RunCriteriaDescriptorOrLabel::Descriptor(criteria_descriptor)) => { - container.run_criteria_label = criteria_descriptor.label; - container.run_criteria_index = - Some(self.add_run_criteria_internal(criteria_descriptor)); - } - None => { - container.run_criteria_index = default_run_criteria; - } - } - self.uninitialized_parallel.push(self.parallel.len()); - self.parallel.push(container); + Some(RunCriteriaDescriptorOrLabel::Descriptor(criteria_descriptor)) => { + container.run_criteria_label = criteria_descriptor.label; + container.run_criteria_index = + Some(self.add_run_criteria_internal(criteria_descriptor)); + } + None => { + container.run_criteria_index = default_run_criteria; } } + self.uninitialized_systems.push((container, system_type)); } pub fn apply_buffers(&mut self, world: &mut World) { @@ -270,53 +226,41 @@ impl SystemStage { pub fn add_system_set(&mut self, system_set: SystemSet) -> &mut Self { self.systems_modified = true; - let (run_criteria, mut systems) = system_set.bake(); - let set_run_criteria_index = run_criteria.and_then(|criteria| { - // validate that no systems have criteria - for system in &mut systems { - if let Some(name) = match system { - SystemDescriptor::Exclusive(descriptor) => descriptor - .run_criteria - .is_some() - .then(|| descriptor.system.name()), - SystemDescriptor::Parallel(descriptor) => descriptor - .run_criteria - .is_some() - .then(|| descriptor.system.name()), - } { - panic!( - "The system {} has a run criteria, but its `SystemSet` also has a run \ - criteria. This is not supported. Consider moving the system into a \ - different `SystemSet` or calling `add_system()` instead.", - name - ) - } + let (run_criteria, mut descriptors) = system_set.bake(); + // verify that none of the systems in the set have their own run criteria + for descriptor in &descriptors { + if let Some(name) = descriptor + .run_criteria + .as_ref() + .map(|_| descriptor.system.name()) + { + panic!( + "The system {} has a run criteria, but its `SystemSet` also has a run \ + criteria. This is not supported. Consider moving the system into a \ + different `SystemSet` or calling `add_system()` instead.", + name + ) } - match criteria { - RunCriteriaDescriptorOrLabel::Descriptor(descriptor) => { - Some(self.add_run_criteria_internal(descriptor)) - } - RunCriteriaDescriptorOrLabel::Label(label) => { - for system in &mut systems { - match system { - SystemDescriptor::Exclusive(descriptor) => { - descriptor.run_criteria = - Some(RunCriteriaDescriptorOrLabel::Label(label)); - } - SystemDescriptor::Parallel(descriptor) => { - descriptor.run_criteria = - Some(RunCriteriaDescriptorOrLabel::Label(label)); - } - } - } + } - None + // add system set run criteria + let set_run_criteria_index = run_criteria.and_then(|criteria| match criteria { + RunCriteriaDescriptorOrLabel::Descriptor(descriptor) => { + Some(self.add_run_criteria_internal(descriptor)) + } + RunCriteriaDescriptorOrLabel::Label(label) => { + for descriptor in &mut descriptors { + descriptor.run_criteria = Some(RunCriteriaDescriptorOrLabel::Label(label)); } + None } }); - for system in systems.drain(..) { - self.add_system_inner(system, set_run_criteria_index); + + // set every system to use the set's run criteria + for descriptor in descriptors.drain(..) { + self.add_system_inner(descriptor, set_run_criteria_index); } + self } @@ -363,30 +307,29 @@ impl SystemStage { let mut criteria_labels = HashMap::default(); let uninitialized_criteria: HashMap<_, _> = self.uninitialized_run_criteria.drain(..).collect(); - // track the number of filtered criteria to correct run criteria indices - let mut filtered_criteria = 0; + + // remove duplicate run criteria instances and alert systems to their new indices + let mut num_duplicates = 0; let mut new_indices = Vec::new(); - self.run_criteria = self - .run_criteria + self.run_criteria = self.run_criteria .drain(..) .enumerate() .filter_map(|(index, mut container)| { - let new_index = index - filtered_criteria; + let new_index = index - num_duplicates; let label = container.label; if let Some(strategy) = uninitialized_criteria.get(&index) { if let Some(ref label) = label { - if let Some(duplicate_index) = criteria_labels.get(label) { + if let Some(&duplicate_index) = criteria_labels.get(label) { match strategy { DuplicateLabelStrategy::Panic => panic!( - "Run criteria {} is labelled with {:?}, which \ - is already in use. Consider using \ - `RunCriteriaDescriptorCoercion::label_discard_if_duplicate().", + "Run criteria {} has label {:?}, which is already in use. \ + Consider using `RunCriteriaDescriptorCoercion::label_discard_if_duplicate().", container.name(), container.label ), DuplicateLabelStrategy::Discard => { - new_indices.push(*duplicate_index); - filtered_criteria += 1; + new_indices.push(duplicate_index); + num_duplicates += 1; return None; } } @@ -402,33 +345,34 @@ impl SystemStage { }) .collect(); - for index in self.uninitialized_at_start.drain(..) { - let container = &mut self.exclusive_at_start[index]; - if let Some(index) = container.run_criteria() { - container.set_run_criteria(new_indices[index]); - } + // initialize the systems and sort them into the appropriate vectors + for (mut container, system_type) in self.uninitialized_systems.drain(..) { container.system_mut().initialize(world); - } - for index in self.uninitialized_before_commands.drain(..) { - let container = &mut self.exclusive_before_commands[index]; - if let Some(index) = container.run_criteria() { - container.set_run_criteria(new_indices[index]); - } - container.system_mut().initialize(world); - } - for index in self.uninitialized_at_end.drain(..) { - let container = &mut self.exclusive_at_end[index]; + if let Some(index) = container.run_criteria() { container.set_run_criteria(new_indices[index]); } - container.system_mut().initialize(world); - } - for index in self.uninitialized_parallel.drain(..) { - let container = &mut self.parallel[index]; - if let Some(index) = container.run_criteria() { - container.set_run_criteria(new_indices[index]); + + match system_type { + SystemType::Parallel => { + if container.system().is_exclusive() { + self.exclusive_at_start.push(container); + } else { + self.parallel.push(container); + } + } + SystemType::Exclusive(insertion_point) => match insertion_point { + InsertionPoint::AtStart => { + self.exclusive_at_start.push(container); + } + InsertionPoint::BeforeCommands => { + self.exclusive_before_commands.push(container); + } + InsertionPoint::AtEnd => { + self.exclusive_at_end.push(container); + } + }, } - container.system_mut().initialize(world); } } @@ -445,11 +389,7 @@ impl SystemStage { < (CHECK_TICK_THRESHOLD as usize) ); debug_assert!( - self.uninitialized_run_criteria.is_empty() - && self.uninitialized_parallel.is_empty() - && self.uninitialized_at_start.is_empty() - && self.uninitialized_before_commands.is_empty() - && self.uninitialized_at_end.is_empty() + self.uninitialized_run_criteria.is_empty() && self.uninitialized_systems.is_empty() ); fn unwrap_dependency_cycle_error( result: Result>, @@ -769,8 +709,8 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec< let a_access = systems[index_a].component_access(); let b_access = systems[index_b].component_access(); if let (Some(a), Some(b)) = (a_access, b_access) { - let conflicts = a.get_conflicts(b); - if !conflicts.is_empty() { + if !a.is_compatible(b) { + let conflicts = a.get_conflicts(b); ambiguities.push((index_a, index_b, conflicts)); } } else { @@ -865,13 +805,25 @@ impl Stage for SystemStage { // Run systems that want to be at the start of stage. for container in &mut self.exclusive_at_start { if should_run(container, &self.run_criteria, default_should_run) { - #[cfg(feature = "trace")] - let _system_span = bevy_utils::tracing::info_span!( - "exclusive_system", - name = &*container.name() - ) - .entered(); - container.system_mut().run(world); + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "exclusive_system", + name = &*container.name() + ) + .entered(); + container.system_mut().run((), world); + } + + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "system_commands", + name = &*container.name() + ) + .entered(); + container.system_mut().apply_buffers(world); + } } } @@ -886,13 +838,25 @@ impl Stage for SystemStage { // Run systems that want to be between parallel systems and their command buffers. for container in &mut self.exclusive_before_commands { if should_run(container, &self.run_criteria, default_should_run) { - #[cfg(feature = "trace")] - let _system_span = bevy_utils::tracing::info_span!( - "exclusive_system", - name = &*container.name() - ) - .entered(); - container.system_mut().run(world); + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "exclusive_system", + name = &*container.name() + ) + .entered(); + container.system_mut().run((), world); + } + + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "system_commands", + name = &*container.name() + ) + .entered(); + container.system_mut().apply_buffers(world); + } } } @@ -914,13 +878,25 @@ impl Stage for SystemStage { // Run systems that want to be at the end of stage. for container in &mut self.exclusive_at_end { if should_run(container, &self.run_criteria, default_should_run) { - #[cfg(feature = "trace")] - let _system_span = bevy_utils::tracing::info_span!( - "exclusive_system", - name = &*container.name() - ) - .entered(); - container.system_mut().run(world); + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "exclusive_system", + name = &*container.name() + ) + .entered(); + container.system_mut().run((), world); + } + + { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "system_commands", + name = &*container.name() + ) + .entered(); + container.system_mut().apply_buffers(world); + } } } @@ -972,11 +948,10 @@ impl Stage for SystemStage { mod tests { use crate::{ schedule::{ - ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, RunCriteria, - RunCriteriaDescriptorCoercion, ShouldRun, SingleThreadedExecutor, Stage, SystemLabel, - SystemLabelId, SystemSet, SystemStage, + IntoSystemDescriptor, RunCriteria, RunCriteriaDescriptorCoercion, ShouldRun, + SingleThreadedExecutor, Stage, SystemLabel, SystemLabelId, SystemSet, SystemStage, }, - system::{In, IntoExclusiveSystem, Local, Query, ResMut}, + system::{In, Local, Query, ResMut}, world::World, }; @@ -1630,6 +1605,7 @@ mod tests { fn empty() {} fn resource(_: ResMut) {} fn component(_: Query<&mut W>) {} + fn exclusive(_: &mut World) {} let mut world = World::new(); @@ -1897,26 +1873,44 @@ mod tests { assert_eq!(ambiguities.len(), 2); let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0")) - .with_system(empty.exclusive_system().label("1").after("0")) - .with_system(empty.exclusive_system().label("2").after("1")) - .with_system(empty.exclusive_system().label("3").after("2")) - .with_system(empty.exclusive_system().label("4").after("3")) - .with_system(empty.exclusive_system().label("5").after("4")) - .with_system(empty.exclusive_system().label("6").after("5")) - .with_system(empty.exclusive_system().label("7").after("6")); + .with_system(exclusive.exclusive_system().label("0")) + .with_system(exclusive.exclusive_system().label("1").after("0")) + .with_system(exclusive.exclusive_system().label("2").after("1")) + .with_system(exclusive.exclusive_system().label("3").after("2")) + .with_system(exclusive.exclusive_system().label("4").after("3")) + .with_system(exclusive.exclusive_system().label("5").after("4")) + .with_system(exclusive.exclusive_system().label("6").after("5")) + .with_system(exclusive.exclusive_system().label("7").after("6")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); assert_eq!(find_ambiguities(&stage.exclusive_at_start).len(), 0); let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0").before("1").before("3")) - .with_system(empty.exclusive_system().label("1")) - .with_system(empty.exclusive_system().label("2").after("1")) - .with_system(empty.exclusive_system().label("3")) - .with_system(empty.exclusive_system().label("4").after("3").before("5")) - .with_system(empty.exclusive_system().label("5")) - .with_system(empty.exclusive_system().label("6").after("2").after("5")); + .with_system( + exclusive + .exclusive_system() + .label("0") + .before("1") + .before("3"), + ) + .with_system(exclusive.exclusive_system().label("1")) + .with_system(exclusive.exclusive_system().label("2").after("1")) + .with_system(exclusive.exclusive_system().label("3")) + .with_system( + exclusive + .exclusive_system() + .label("4") + .after("3") + .before("5"), + ) + .with_system(exclusive.exclusive_system().label("5")) + .with_system( + exclusive + .exclusive_system() + .label("6") + .after("2") + .after("5"), + ); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start); @@ -1947,13 +1941,46 @@ mod tests { assert_eq!(ambiguities.len(), 6); let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0").before("1").before("3")) - .with_system(empty.exclusive_system().label("1").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("2").after("1")) - .with_system(empty.exclusive_system().label("3").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("4").after("3").before("5")) - .with_system(empty.exclusive_system().label("5").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("6").after("2").after("5")); + .with_system( + exclusive + .exclusive_system() + .label("0") + .before("1") + .before("3"), + ) + .with_system( + exclusive + .exclusive_system() + .label("1") + .in_ambiguity_set("a"), + ) + .with_system(exclusive.exclusive_system().label("2").after("1")) + .with_system( + exclusive + .exclusive_system() + .label("3") + .in_ambiguity_set("a"), + ) + .with_system( + exclusive + .exclusive_system() + .label("4") + .after("3") + .before("5"), + ) + .with_system( + exclusive + .exclusive_system() + .label("5") + .in_ambiguity_set("a"), + ) + .with_system( + exclusive + .exclusive_system() + .label("6") + .after("2") + .after("5"), + ); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start); @@ -1976,10 +2003,30 @@ mod tests { assert_eq!(ambiguities.len(), 4); let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("1").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("2").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("3").in_ambiguity_set("a")); + .with_system( + exclusive + .exclusive_system() + .label("0") + .in_ambiguity_set("a"), + ) + .with_system( + exclusive + .exclusive_system() + .label("1") + .in_ambiguity_set("a"), + ) + .with_system( + exclusive + .exclusive_system() + .label("2") + .in_ambiguity_set("a"), + ) + .with_system( + exclusive + .exclusive_system() + .label("3") + .in_ambiguity_set("a"), + ); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start); diff --git a/crates/bevy_ecs/src/schedule/system_container.rs b/crates/bevy_ecs/src/schedule/system_container.rs index 51d0962489143..5c9319df2788d 100644 --- a/crates/bevy_ecs/src/schedule/system_container.rs +++ b/crates/bevy_ecs/src/schedule/system_container.rs @@ -2,10 +2,9 @@ use crate::{ component::ComponentId, query::Access, schedule::{ - AmbiguitySetLabelId, ExclusiveSystemDescriptor, GraphNode, ParallelSystemDescriptor, - RunCriteriaLabelId, SystemLabelId, + AmbiguitySetLabelId, GraphNode, RunCriteriaLabelId, SystemDescriptor, SystemLabelId, }, - system::{ExclusiveSystem, System}, + system::System, }; use std::borrow::Cow; @@ -24,88 +23,7 @@ pub trait SystemContainer: GraphNode