Mirrors/bevy
2
0
Fork 0
mirror of https://github.com/bevyengine/bevy synced 2024-09-20 06:22:01 +00:00
Code Issues Projects Releases Packages Wiki Activity

Improve safety for the multi-threaded executor using UnsafeWorldCell (#8292)

Browse source 
# Objective

Fix #7833.

Safety comments in the multi-threaded executor don't really talk about
system world accesses, which makes it unclear if the code is actually
valid.

## Solution

Update the `System` trait to use `UnsafeWorldCell`. This type's API is
written in a way that makes it much easier to cleanly maintain safety
invariants. Use this type throughout the multi-threaded executor, with a
liberal use of safety comments.

---

## Migration Guide

The `System` trait now uses `UnsafeWorldCell` instead of `&World`. This
type provides a robust API for interior mutable world access.
- The method `run_unsafe` uses this type to manage world mutations
across multiple threads.
- The method `update_archetype_component_access` uses this type to
ensure that only world metadata can be used.

```rust
let mut system = IntoSystem::into_system(my_system);
system.initialize(&mut world);

// Before:
system.update_archetype_component_access(&world);
unsafe { system.run_unsafe(&world) }

// After:
system.update_archetype_component_access(world.as_unsafe_world_cell_readonly());
unsafe { system.run_unsafe(world.as_unsafe_world_cell()) }
```

---------

Co-authored-by: James Liu <contact@jamessliu.com>
This commit is contained in:
JoJoJet 2023-05-29 11:22:10 -04:00 committed by GitHub
parent 4465f256eb
commit 85a918a8dd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 103 additions and 69 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
benches/benches/bevy_ecs/iteration/heavy_compute.rs
View file

@ -45,7 +45,7 @@ pub fn heavy_compute(c: &mut Criterion) {
let mut system = IntoSystem::into_system(sys);
system.initialize(&mut world);
system.update_archetype_component_access(&world);
system.update_archetype_component_access(world.as_unsafe_world_cell());
b.iter(move || system.run((), &mut world));
});

2
benches/benches/bevy_ecs/iteration/iter_simple_system.rs
View file

@ -37,7 +37,7 @@ impl Benchmark {
let mut system = IntoSystem::into_system(query_system);
system.initialize(&mut world);
system.update_archetype_component_access(&world);
system.update_archetype_component_access(world.as_unsafe_world_cell());
Self(world, Box::new(system))
}

2
benches/benches/bevy_ecs/world/world_get.rs
View file

@ -291,7 +291,7 @@ pub fn query_get_component_simple(criterion: &mut Criterion) {
let mut system = IntoSystem::into_system(query_system);
system.initialize(&mut world);
system.update_archetype_component_access(&world);
system.update_archetype_component_access(world.as_unsafe_world_cell());
bencher.iter(|| system.run(entity, &mut world));
});

5
crates/bevy_ecs/src/schedule/condition.rs
View file

@ -5,6 +5,7 @@ use std::ops::Not;
use crate::component::{self, ComponentId};
use crate::query::Access;
use crate::system::{CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System};
use crate::world::unsafe_world_cell::UnsafeWorldCell;
use crate::world::World;
pub type BoxedCondition = Box<dyn ReadOnlySystem<In = (), Out = bool>>;
@ -990,7 +991,7 @@ where
self.condition.is_exclusive()
}
unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out {
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out {
// SAFETY: The inner condition system asserts its own safety.
!self.condition.run_unsafe(input, world)
}
@ -1007,7 +1008,7 @@ where
self.condition.initialize(world);
}
fn update_archetype_component_access(&mut self, world: &World) {
fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) {
self.condition.update_archetype_component_access(world);
}

89
crates/bevy_ecs/src/schedule/executor/multi_threaded.rs
View file

@ -21,7 +21,7 @@ use crate::{
is_apply_system_buffers, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
},
system::BoxedSystem,
world::World,
world::{unsafe_world_cell::UnsafeWorldCell, World},
};
use crate as bevy_ecs;
@ -184,7 +184,6 @@ impl SystemExecutor for MultiThreadedExecutor {
.map(|e| e.0.clone());
let thread_executor = thread_executor.as_deref();
let world = SyncUnsafeCell::from_mut(world);
let SyncUnsafeSchedule {
systems,
mut conditions,
@ -197,10 +196,13 @@ impl SystemExecutor for MultiThreadedExecutor {
// the executor itself is a `Send` future so that it can run
// alongside systems that claim the local thread
let executor = async {
let world_cell = world.as_unsafe_world_cell();
while self.num_completed_systems < self.num_systems {
// SAFETY: self.ready_systems does not contain running systems
// SAFETY:
// - self.ready_systems does not contain running systems.
// - `world_cell` has mutable access to the entire world.
unsafe {
self.spawn_system_tasks(scope, systems, &mut conditions, world);
self.spawn_system_tasks(scope, systems, &mut conditions, world_cell);
}
if self.num_running_systems > 0 {
@ -231,7 +233,7 @@ impl SystemExecutor for MultiThreadedExecutor {
if self.apply_final_buffers {
// Do one final apply buffers after all systems have completed
// Commands should be applied while on the scope's thread, not the executor's thread
let res = apply_system_buffers(&self.unapplied_systems, systems, world.get_mut());
let res = apply_system_buffers(&self.unapplied_systems, systems, world);
if let Err(payload) = res {
let mut panic_payload = self.panic_payload.lock().unwrap();
*panic_payload = Some(payload);
@ -283,14 +285,16 @@ impl MultiThreadedExecutor {
}
/// # Safety
/// Caller must ensure that `self.ready_systems` does not contain any systems that
/// have been mutably borrowed (such as the systems currently running).
/// - Caller must ensure that `self.ready_systems` does not contain any systems that
/// have been mutably borrowed (such as the systems currently running).
/// - `world_cell` must have permission to access all world data (not counting
/// any world data that is claimed by systems currently running on this executor).
unsafe fn spawn_system_tasks<'scope>(
&mut self,
scope: &Scope<'_, 'scope, ()>,
systems: &'scope [SyncUnsafeCell<BoxedSystem>],
conditions: &mut Conditions,
cell: &'scope SyncUnsafeCell<World>,
world_cell: UnsafeWorldCell<'scope>,
) {
if self.exclusive_running {
return;
@ -307,10 +311,7 @@ impl MultiThreadedExecutor {
// Therefore, no other reference to this system exists and there is no aliasing.
let system = unsafe { &mut *systems[system_index].get() };
// SAFETY: No exclusive system is running.
// Therefore, there is no existing mutable reference to the world.
let world = unsafe { &*cell.get() };
if !self.can_run(system_index, system, conditions, world) {
if !self.can_run(system_index, system, conditions, world_cell) {
// NOTE: exclusive systems with ambiguities are susceptible to
// being significantly displaced here (compared to single-threaded order)
// if systems after them in topological order can run
@ -320,9 +321,10 @@ impl MultiThreadedExecutor {
self.ready_systems.set(system_index, false);
// SAFETY: Since `self.can_run` returned true earlier, it must have called
// `update_archetype_component_access` for each run condition.
if !self.should_run(system_index, system, conditions, world) {
// SAFETY: `can_run` returned true, which means that:
// - It must have called `update_archetype_component_access` for each run condition.
// - There can be no systems running whose accesses would conflict with any conditions.
if !self.should_run(system_index, system, conditions, world_cell) {
self.skip_system_and_signal_dependents(system_index);
continue;
}
@ -331,10 +333,12 @@ impl MultiThreadedExecutor {
self.num_running_systems += 1;
if self.system_task_metadata[system_index].is_exclusive {
// SAFETY: `can_run` confirmed that no systems are running.
// Therefore, there is no existing reference to the world.
// SAFETY: `can_run` returned true for this system, which means
// that no other systems currently have access to the world.
let world = unsafe { world_cell.world_mut() };
// SAFETY: `can_run` returned true for this system,
// which means no systems are currently borrowed.
unsafe {
let world = &mut *cell.get();
self.spawn_exclusive_system_task(scope, system_index, systems, world);
}
break;
@ -342,9 +346,10 @@ impl MultiThreadedExecutor {
// SAFETY:
// - No other reference to this system exists.
// - `self.can_run` has been called, which calls `update_archetype_component_access` with this system.
// - `can_run` has been called, which calls `update_archetype_component_access` with this system.
// - `can_run` returned true, so no systems with conflicting world access are running.
unsafe {
self.spawn_system_task(scope, system_index, systems, world);
self.spawn_system_task(scope, system_index, systems, world_cell);
}
}
@ -357,7 +362,7 @@ impl MultiThreadedExecutor {
system_index: usize,
system: &mut BoxedSystem,
conditions: &mut Conditions,
world: &World,
world: UnsafeWorldCell,
) -> bool {
let system_meta = &self.system_task_metadata[system_index];
if system_meta.is_exclusive && self.num_running_systems > 0 {
@ -413,15 +418,17 @@ impl MultiThreadedExecutor {
}
/// # Safety
///
/// `update_archetype_component` must have been called with `world`
/// for each run condition in `conditions`.
/// * `world` must have permission to read any world data required by
/// the system's conditions: this includes conditions for the system
/// itself, and conditions for any of the system's sets.
/// * `update_archetype_component` must have been called with `world`
/// for each run condition in `conditions`.
unsafe fn should_run(
&mut self,
system_index: usize,
_system: &BoxedSystem,
conditions: &mut Conditions,
world: &World,
world: UnsafeWorldCell,
) -> bool {
let mut should_run = !self.skipped_systems.contains(system_index);
for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() {
@ -430,7 +437,10 @@ impl MultiThreadedExecutor {
}
// Evaluate the system set's conditions.
// SAFETY: `update_archetype_component_access` has been called for each run condition.
// SAFETY:
// - The caller ensures that `world` has permission to read any data
// required by the conditions.
// - `update_archetype_component_access` has been called for each run condition.
let set_conditions_met =
evaluate_and_fold_conditions(&mut conditions.set_conditions[set_idx], world);
@ -444,7 +454,10 @@ impl MultiThreadedExecutor {
}
// Evaluate the system's conditions.
// SAFETY: `update_archetype_component_access` has been called for each run condition.
// SAFETY:
// - The caller ensures that `world` has permission to read any data
// required by the conditions.
// - `update_archetype_component_access` has been called for each run condition.
let system_conditions_met =
evaluate_and_fold_conditions(&mut conditions.system_conditions[system_index], world);
@ -459,6 +472,8 @@ impl MultiThreadedExecutor {
/// # Safety
/// - Caller must not alias systems that are running.
/// - `world` must have permission to access the world data
/// used by the specified system.
/// - `update_archetype_component_access` must have been called with `world`
/// on the system assocaited with `system_index`.
unsafe fn spawn_system_task<'scope>(
@ -466,7 +481,7 @@ impl MultiThreadedExecutor {
scope: &Scope<'_, 'scope, ()>,
system_index: usize,
systems: &'scope [SyncUnsafeCell<BoxedSystem>],
world: &'scope World,
world: UnsafeWorldCell<'scope>,
) {
// SAFETY: this system is not running, no other reference exists
let system = unsafe { &mut *systems[system_index].get() };
@ -483,7 +498,8 @@ impl MultiThreadedExecutor {
let system_guard = system_span.enter();
let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
// SAFETY:
// - Access: TODO.
// - The caller ensures that we have permission to
// access the world data used by the system.
// - `update_archetype_component_access` has been called.
unsafe { system.run_unsafe((), world) };
}));
@ -688,10 +704,14 @@ fn apply_system_buffers(
}
/// # Safety
///
/// `update_archetype_component_access` must have been called
/// with `world` for each condition in `conditions`.
unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool {
/// - `world` must have permission to read any world data
/// required by `conditions`.
/// - `update_archetype_component_access` must have been called
/// with `world` for each condition in `conditions`.
unsafe fn evaluate_and_fold_conditions(
conditions: &mut [BoxedCondition],
world: UnsafeWorldCell,
) -> bool {
// not short-circuiting is intentional
#[allow(clippy::unnecessary_fold)]
conditions
@ -699,7 +719,8 @@ unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world:
.map(|condition| {
#[cfg(feature = "trace")]
let _condition_span = info_span!("condition", name = &*condition.name()).entered();
// SAFETY: caller ensures system access is compatible
// SAFETY: The caller ensures that `world` has permission to
// access any data required by the condition.
unsafe { condition.run_unsafe((), world) }
})
.fold(true, |acc, res| acc && res)

5
crates/bevy_ecs/src/system/combinator.rs
View file

@ -7,6 +7,7 @@ use crate::{
component::{ComponentId, Tick},
prelude::World,
query::Access,
world::unsafe_world_cell::UnsafeWorldCell,
};
use super::{ReadOnlySystem, System};
@ -157,7 +158,7 @@ where
self.a.is_exclusive() || self.b.is_exclusive()
}
unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out {
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out {
Func::combine(
input,
// SAFETY: The world accesses for both underlying systems have been registered,
@ -198,7 +199,7 @@ where
self.component_access.extend(self.b.component_access());
}
fn update_archetype_component_access(&mut self, world: &World) {
fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) {
self.a.update_archetype_component_access(world);
self.b.update_archetype_component_access(world);

6
crates/bevy_ecs/src/system/exclusive_function_system.rs
View file

@ -6,7 +6,7 @@ use crate::{
check_system_change_tick, ExclusiveSystemParam, ExclusiveSystemParamItem, In, IntoSystem,
System, SystemMeta,
},
world::World,
world::{unsafe_world_cell::UnsafeWorldCell, World},
};
use bevy_utils::all_tuples;
@ -86,7 +86,7 @@ where
}
#[inline]
unsafe fn run_unsafe(&mut self, _input: Self::In, _world: &World) -> Self::Out {
unsafe fn run_unsafe(&mut self, _input: Self::In, _world: UnsafeWorldCell) -> Self::Out {
panic!("Cannot run exclusive systems with a shared World reference");
}
@ -134,7 +134,7 @@ where
self.param_state = Some(F::Param::init(world, &mut self.system_meta));
}
fn update_archetype_component_access(&mut self, _world: &World) {}
fn update_archetype_component_access(&mut self, _world: UnsafeWorldCell) {}
#[inline]
fn check_change_tick(&mut self, change_tick: Tick) {

8
crates/bevy_ecs/src/system/function_system.rs
View file

@ -4,7 +4,7 @@ use crate::{
prelude::FromWorld,
query::{Access, FilteredAccessSet},
system::{check_system_change_tick, ReadOnlySystemParam, System, SystemParam, SystemParamItem},
world::{World, WorldId},
world::{unsafe_world_cell::UnsafeWorldCell, World, WorldId},
};
use bevy_utils::all_tuples;
@ -417,7 +417,7 @@ where
}
#[inline]
unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out {
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out {
let change_tick = world.increment_change_tick();
// SAFETY:
@ -428,7 +428,7 @@ where
let params = F::Param::get_param(
self.param_state.as_mut().expect(Self::PARAM_MESSAGE),
&self.system_meta,
world.as_unsafe_world_cell_migration_internal(),
world,
change_tick,
);
let out = self.func.run(input, params);
@ -457,7 +457,7 @@ where
self.param_state = Some(F::Param::init_state(world, &mut self.system_meta));
}
fn update_archetype_component_access(&mut self, world: &World) {
fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) {
assert!(self.world_id == Some(world.id()), "Encountered a mismatched World. A System cannot be used with Worlds other than the one it was initialized with.");
let archetypes = world.archetypes();
let new_generation = archetypes.generation();

6
crates/bevy_ecs/src/system/mod.rs
View file

@ -1610,7 +1610,7 @@ mod tests {
// set up system and verify its access is empty
system.initialize(&mut world);
system.update_archetype_component_access(&world);
system.update_archetype_component_access(world.as_unsafe_world_cell());
assert_eq!(
system
.archetype_component_access()
@ -1640,7 +1640,7 @@ mod tests {
world.spawn((B, C));
// update system and verify its accesses are correct
system.update_archetype_component_access(&world);
system.update_archetype_component_access(world.as_unsafe_world_cell());
assert_eq!(
system
.archetype_component_access()
@ -1658,7 +1658,7 @@ mod tests {
.unwrap(),
);
world.spawn((A, B, D));
system.update_archetype_component_access(&world);
system.update_archetype_component_access(world.as_unsafe_world_cell());
assert_eq!(
system
.archetype_component_access()

31
crates/bevy_ecs/src/system/system.rs
View file

@ -2,6 +2,7 @@ use bevy_utils::tracing::warn;
use core::fmt::Debug;
use crate::component::Tick;
use crate::world::unsafe_world_cell::UnsafeWorldCell;
use crate::{archetype::ArchetypeComponentId, component::ComponentId, query::Access, world::World};
use std::any::TypeId;
@ -39,26 +40,24 @@ pub trait System: Send + Sync + 'static {
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.
/// can be called in parallel with other systems and may break Rust's aliasing rules
/// if used incorrectly, making it unsafe to call.
///
/// # Safety
///
/// This might access world and resources in an unsafe manner. This should only be called in one
/// of the following contexts:
/// 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()`].
///
/// Additionally, the method [`Self::update_archetype_component_access`] must be called at some
/// point before this one, with the same exact [`World`]. If `update_archetype_component_access`
/// panics (or otherwise does not return for any reason), this method must not be called.
unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out;
/// - The caller must ensure that `world` has permission to access any world data
/// registered in [`Self::archetype_component_access`]. There must be no conflicting
/// simultaneous accesses while the system is running.
/// - The method [`Self::update_archetype_component_access`] must be called at some
/// point before this one, with the same exact [`World`]. If `update_archetype_component_access`
/// panics (or otherwise does not return for any reason), this method must not be called.
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out;
/// Runs the system with the given input in the world.
fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out {
let world = world.as_unsafe_world_cell();
self.update_archetype_component_access(world);
// SAFETY:
// - World and resources are exclusively borrowed, which ensures no data access conflicts.
// - We have exclusive access to the entire world.
// - `update_archetype_component_access` has been called.
unsafe { self.run_unsafe(input, world) }
}
@ -66,7 +65,11 @@ pub trait System: Send + Sync + 'static {
/// Initialize the system.
fn initialize(&mut self, _world: &mut World);
/// Update the system's archetype component [`Access`].
fn update_archetype_component_access(&mut self, world: &World);
///
/// ## Note for implementors
/// `world` may only be used to access metadata. This can be done in safe code
/// via functions such as [`UnsafeWorldCell::archetypes`].
fn update_archetype_component_access(&mut self, world: UnsafeWorldCell);
fn check_change_tick(&mut self, change_tick: Tick);
/// Returns the system's default [system sets](crate::schedule::SystemSet).
fn default_system_sets(&self) -> Vec<Box<dyn crate::schedule::SystemSet>> {

6
crates/bevy_ecs/src/world/identifier.rs
View file

@ -44,10 +44,10 @@ impl FromWorld for WorldId {
}
}
// SAFETY: Has read-only access to shared World metadata
// SAFETY: No world data is accessed.
unsafe impl ReadOnlySystemParam for WorldId {}
// SAFETY: A World's ID is immutable and fetching it from the World does not borrow anything
// SAFETY: No world data is accessed.
unsafe impl SystemParam for WorldId {
type State = ();
@ -61,7 +61,7 @@ unsafe impl SystemParam for WorldId {
world: UnsafeWorldCell<'world>,
_: Tick,
) -> Self::Item<'world, 'state> {
world.world_metadata().id()
world.id()
}
}

10
crates/bevy_ecs/src/world/unsafe_world_cell.rs
View file

@ -1,6 +1,6 @@
#![warn(unsafe_op_in_unsafe_fn)]
use super::{Mut, World};
use super::{Mut, World, WorldId};
use crate::{
archetype::{Archetype, ArchetypeComponentId, Archetypes},
bundle::Bundles,
@ -190,6 +190,14 @@ impl<'w> UnsafeWorldCell<'w> {
unsafe { &*self.0 }
}
/// Retrieves this world's unique [ID](WorldId).
#[inline]
pub fn id(self) -> WorldId {
// SAFETY:
// - we only access world metadata
unsafe { self.world_metadata() }.id()
}
/// Retrieves this world's [Entities] collection
#[inline]
pub fn entities(self) -> &'w Entities {