Optimize param validation through new try_get_param

crates/bevy_ecs/src/lib.rs

@@ -60,7 +60,7 @@ pub mod prelude {
             Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef, IntoSystem, Local,
             NonSend, NonSendMut, ParallelCommands, ParamSet, Populated, Query, ReadOnlySystem, Res,
             ResMut, Resource, Single, System, SystemIn, SystemInput, SystemParamBuilder,
-            SystemParamFunction,
+            SystemParamFunction, WithParamWarnPolicy,
         },
         world::{
             Command, EntityMut, EntityRef, EntityWorldMut, FromWorld, OnAdd, OnInsert, OnRemove,

crates/bevy_ecs/src/observer/runner.rs

@@ -374,8 +374,8 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     // - system is the same type erased system from above
     unsafe {
         (*system).update_archetype_component_access(world);
-        if (*system).validate_param_unsafe(world) {
-            (*system).run_unsafe(trigger, world);
+        let did_run = (*system).try_run_unsafe(trigger, world).is_ok();
+        if did_run {
             (*system).queue_deferred(world.into_deferred());
         }
     }

crates/bevy_ecs/src/schedule/executor/mod.rs

@@ -140,58 +140,74 @@ mod __rust_begin_short_backtrace {
     use core::hint::black_box;
 
     use crate::{
-        system::{ReadOnlySystem, System},
+        system::{ReadOnlySystem, RunSystemError, System},
         world::{unsafe_world_cell::UnsafeWorldCell, World},
     };
 
+    /// Run system without validating params.
+    /// Does not apply deferred parameters.
+    ///
     /// # Safety
-    /// See `System::run_unsafe`.
+    ///
+    /// - System params were validated and no changes in accessed world happened since.
+    /// - Same as [`System::run_unsafe`].
     #[inline(never)]
     pub(super) unsafe fn run_unsafe(
         system: &mut dyn System<In = (), Out = ()>,
         world: UnsafeWorldCell,
     ) {
-        system.run_unsafe((), world);
-        black_box(());
+        black_box(system.run_unsafe((), world));
     }
 
+    /// Run system with param validation.
+    /// Does not apply deferred parameters.
+    /// Returns whether system was executed.
+    ///
     /// # Safety
-    /// See `ReadOnlySystem::run_unsafe`.
+    ///
+    /// - Same as [`System::run_unsafe`].
     #[inline(never)]
-    pub(super) unsafe fn readonly_run_unsafe<O: 'static>(
-        system: &mut dyn ReadOnlySystem<In = (), Out = O>,
+    pub(super) unsafe fn try_run_unsafe(
+        system: &mut dyn System<In = (), Out = ()>,
         world: UnsafeWorldCell,
-    ) -> O {
-        black_box(system.run_unsafe((), world))
+    ) {
+        _ = black_box(system.try_run_unsafe((), world));
+    }
+
+    /// Run system with param validation.
+    /// Applies deferred parameters.
+    #[inline(never)]
+    pub(super) fn try_run(system: &mut dyn System<In = (), Out = ()>, world: &mut World) {
+        _ = black_box(system.try_run((), world));
     }
 
+    /// Run readonly system with param validation.
+    /// Does not apply deferred parameters.
+    /// Returns [`Some`] if system was executed and [`None`] otherwise.
+    ///
+    /// # Safety
+    ///
+    /// - Same as [`System::run_unsafe`].
     #[inline(never)]
-    pub(super) fn run(system: &mut dyn System<In = (), Out = ()>, world: &mut World) {
-        system.run((), world);
-        black_box(());
+    pub(super) unsafe fn try_readonly_run_unsafe<O: 'static>(
+        system: &mut dyn ReadOnlySystem<In = (), Out = O>,
+        world: UnsafeWorldCell,
+    ) -> Result<O, RunSystemError> {
+        black_box(system.try_run_unsafe((), world))
     }
 
+    /// Run readonly system with param validation.
+    /// Applies deferred parameters.
+    /// Returns [`Some`] if system was executed and [`None`] otherwise.
     #[inline(never)]
-    pub(super) fn readonly_run<O: 'static>(
+    pub(super) fn try_readonly_run<O: 'static>(
         system: &mut dyn ReadOnlySystem<In = (), Out = O>,
         world: &mut World,
-    ) -> O {
-        black_box(system.run((), world))
+    ) -> Result<O, RunSystemError> {
+        black_box(system.try_run((), world))
     }
 }
 
-#[macro_export]
-/// Emits a warning about system being skipped.
-macro_rules! warn_system_skipped {
-    ($ty:literal, $sys:expr) => {
-        bevy_utils::tracing::warn!(
-            "{} {} was skipped due to inaccessible system parameters.",
-            $ty,
-            $sys
-        )
-    };
-}
-
 #[cfg(test)]
 mod tests {
     use crate::{

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

@@ -19,7 +19,6 @@ use crate::{
     query::Access,
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
     system::BoxedSystem,
-    warn_system_skipped,
     world::{unsafe_world_cell::UnsafeWorldCell, World},
 };
 
@@ -524,7 +523,7 @@ impl ExecutorState {
     unsafe fn should_run(
         &mut self,
         system_index: usize,
-        system: &BoxedSystem,
+        system: &mut BoxedSystem,
         conditions: &mut Conditions,
         world: UnsafeWorldCell,
     ) -> bool {
@@ -568,14 +567,14 @@ impl ExecutorState {
 
         should_run &= system_conditions_met;
 
+        // Check param validity here to prevent system task from spawning.
         if should_run {
             // SAFETY:
             // - The caller ensures that `world` has permission to read any data
             //   required by the system.
             // - `update_archetype_component_access` has been called for system.
             let valid_params = unsafe { system.validate_param_unsafe(world) };
             if !valid_params {
-                warn_system_skipped!("System", system.name());
                 self.skipped_systems.insert(system_index);
             }
             should_run &= valid_params;
@@ -648,11 +647,16 @@ impl ExecutorState {
             context.scope.spawn_on_scope(task);
         } else {
             let task = async move {
-                // SAFETY: `can_run` returned true for this system, which means
-                // that no other systems currently have access to the world.
-                let world = unsafe { context.environment.world_cell.world_mut() };
                 let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
-                    __rust_begin_short_backtrace::run(&mut **system, world);
+                    // SAFETY:
+                    // - The caller ensures that we have permission to access the entire world.
+                    // - `update_archetype_component_access` has been called.
+                    unsafe {
+                        __rust_begin_short_backtrace::run_unsafe(
+                            &mut **system,
+                            context.environment.world_cell,
+                        )
+                    };
                 }));
                 context.system_completed(system_index, res, system);
             };
@@ -750,15 +754,10 @@ unsafe fn evaluate_and_fold_conditions(
             // - The caller ensures that `world` has permission to read any data
             //   required by the condition.
             // - `update_archetype_component_access` has been called for condition.
-            if !unsafe { condition.validate_param_unsafe(world) } {
-                warn_system_skipped!("Condition", condition.name());
-                return false;
+            unsafe {
+                __rust_begin_short_backtrace::try_readonly_run_unsafe(&mut **condition, world)
             }
-            // SAFETY:
-            // - The caller ensures that `world` has permission to read any data
-            //   required by the condition.
-            // - `update_archetype_component_access` has been called for condition.
-            unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) }
+            .unwrap_or(false)
         })
         .fold(true, |acc, res| acc && res)
 }

crates/bevy_ecs/src/schedule/executor/simple.rs

@@ -7,7 +7,6 @@ use crate::{
     schedule::{
         executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
     },
-    warn_system_skipped,
     world::World,
 };
 
@@ -80,15 +79,6 @@ impl SystemExecutor for SimpleExecutor {
 
             should_run &= system_conditions_met;
 
-            let system = &mut schedule.systems[system_index];
-            if should_run {
-                let valid_params = system.validate_param(world);
-                if !valid_params {
-                    warn_system_skipped!("System", system.name());
-                }
-                should_run &= valid_params;
-            }
-
             #[cfg(feature = "trace")]
             should_run_span.exit();
 
@@ -99,12 +89,13 @@ impl SystemExecutor for SimpleExecutor {
                 continue;
             }
 
+            let system = &mut schedule.systems[system_index];
             if is_apply_deferred(system) {
                 continue;
             }
 
             let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
-                __rust_begin_short_backtrace::run(&mut **system, world);
+                __rust_begin_short_backtrace::try_run(&mut **system, world);
             }));
             if let Err(payload) = res {
                 eprintln!("Encountered a panic in system `{}`!", &*system.name());
@@ -138,11 +129,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
     conditions
         .iter_mut()
         .map(|condition| {
-            if !condition.validate_param(world) {
-                warn_system_skipped!("Condition", condition.name());
-                return false;
-            }
-            __rust_begin_short_backtrace::readonly_run(&mut **condition, world)
+            __rust_begin_short_backtrace::try_readonly_run(&mut **condition, world).unwrap_or(false)
         })
         .fold(true, |acc, res| acc && res)
 }

crates/bevy_ecs/src/schedule/executor/single_threaded.rs

@@ -5,7 +5,6 @@ use fixedbitset::FixedBitSet;
 
 use crate::{
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
-    warn_system_skipped,
     world::World,
 };
 
@@ -86,15 +85,6 @@ impl SystemExecutor for SingleThreadedExecutor {
 
             should_run &= system_conditions_met;
 
-            let system = &mut schedule.systems[system_index];
-            if should_run {
-                let valid_params = system.validate_param(world);
-                if !valid_params {
-                    warn_system_skipped!("System", system.name());
-                }
-                should_run &= valid_params;
-            }
-
             #[cfg(feature = "trace")]
             should_run_span.exit();
 
@@ -105,21 +95,22 @@ impl SystemExecutor for SingleThreadedExecutor {
                 continue;
             }
 
+            let system = &mut schedule.systems[system_index];
             if is_apply_deferred(system) {
                 self.apply_deferred(schedule, world);
                 continue;
             }
 
             let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
                 if system.is_exclusive() {
-                    __rust_begin_short_backtrace::run(&mut **system, world);
+                    __rust_begin_short_backtrace::try_run(&mut **system, world);
                 } else {
                     // Use run_unsafe to avoid immediately applying deferred buffers
                     let world = world.as_unsafe_world_cell();
                     system.update_archetype_component_access(world);
                     // SAFETY: We have exclusive, single-threaded access to the world and
                     // update_archetype_component_access is being called immediately before this.
-                    unsafe { __rust_begin_short_backtrace::run_unsafe(&mut **system, world) };
+                    unsafe { __rust_begin_short_backtrace::try_run_unsafe(&mut **system, world) };
                 }
             }));
             if let Err(payload) = res {
@@ -170,11 +161,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
     conditions
         .iter_mut()
         .map(|condition| {
-            if !condition.validate_param(world) {
-                warn_system_skipped!("Condition", condition.name());
-                return false;
-            }
-            __rust_begin_short_backtrace::readonly_run(&mut **condition, world)
+            __rust_begin_short_backtrace::try_readonly_run(&mut **condition, world).unwrap_or(false)
         })
         .fold(true, |acc, res| acc && res)
 }

crates/bevy_ecs/src/system/adapter_system.rs

@@ -179,8 +179,9 @@ where
     }
 
     #[inline]
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
-        self.system.validate_param_unsafe(world)
+    unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
+        // SAFETY: Delegate to other `System` implementations.
+        unsafe { self.system.validate_param_unsafe(world) }
     }
 
     fn initialize(&mut self, world: &mut crate::prelude::World) {

crates/bevy_ecs/src/system/combinator.rs

@@ -212,8 +212,9 @@ where
     }
 
     #[inline]
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
-        self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world)
+    unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
+        // SAFETY: Delegate to other `System` implementations.
+        unsafe { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) }
     }
 
     fn initialize(&mut self, world: &mut World) {
@@ -430,8 +431,9 @@ where
         self.b.queue_deferred(world);
     }
 
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
-        self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world)
+    unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
+        // SAFETY: Delegate to other `System` implementations.
+        unsafe { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) }
     }
 
     fn validate_param(&mut self, world: &World) -> bool {

crates/bevy_ecs/src/system/exclusive_function_system.rs

@@ -150,7 +150,8 @@ where
     }
 
     #[inline]
-    unsafe fn validate_param_unsafe(&self, _world: UnsafeWorldCell) -> bool {
+    unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool {
+        // All exclusive system params are always available.
         true
     }