Merge `SystemParam::validate_param` into `SystemParam::get_param` by alice-i-cecile · Pull Request #23225 · bevyengine/bevy

alice-i-cecile · 2026-03-04T22:49:49Z

Objective

As raised in #23174, we currently duplicate working when looking up our system parameters: once during validation, and then again when actually fetching the data.

This is (maybe) slow, and would worsen the performance regression incurred by resources-as-components (#19731).

This strategy also imposes some non-trivial complexity and maintainability costs. Because "validate" is a distinct step from "use", it's possible to skip validation! As far as I could tell, this is the case in a number of places before this PR: particularly in the unconventional "please just run my system" path. While in most cases this will simply result in a crash in a different place, it causes these paths to not handle

Fixes #23179. Fixes #15505.

Solution

Fundamentally, what we're doing is rolling the SystemParam::validate_param behavior into SystemParam::get_param, by making the latter return a Result.

However, there is a tremendous amount of splash damage required to get that to actually compile and expose the correct semantics. The most important of these are:

SystemState::get and friends now returns a Result
- this leads to a fair bit of assorted unwrap spam in our tests and weird internal usages
  - these tests can probably be refactored to not use SystemState directly in the future now that we have better tools like run_system_once, but eh, not this PR's job
- this is semantically correct, as these params could fail validation
System::validate_param_unsafe has been removed, and validation now occurs inside of System::run_unsafe
- very much a net positive for both abstract robustness and current correctness
- this impacts the strategy that various executors use: see the next section

There are a lot of moving parts here: I'm sorry that I couldn't get this into a smaller, more incremental refactor. When reviewing this PR, you should begin with the migration guide to help get you oriented on the details: validation_merging.md.

From there, the most important files to review are:

system_param.rs: trait changes and implementers
function_system.rs: primary implementer of System
multithreaded.rs: the parallel executor

NOTE TO REVIEWERS: Please make comments to generate threads; this PR review might get fairly hairy.

Performance discussion

For the parallel MultithreadedExecutor, validation was previously done as a cheap pre-validation step,
while checking run conditions.
Now, tasks will be spawned for systems which would fail or are skipped during validation.

In most cases, avoiding the extra overhead of looking up the required data twice should dominate.
However, this change may negatively affect systems which are frequently skipped (e.g. due to Single).

Paths not taken

In this PR, I've decided not to:

Add another variant RunSystemError, distinguishing "validation failed" from "system ran but returned an error".
- While reusing RunSystemError::Failed for both cases is messy, this PR is already a bit of a nightmare to review.
Return a result from ParamSet::get_mut.
- Instead, we just unwrap.
- Bubbling up the Result is technically more correct, but these were already panicking before if e.g. a resource is missing, and ParamSet is already an ergonomic abomination.

Testing

I've added a number of new tests to exercise the system param validation paths, ensuring that validation is done when systems are run.

However, I would appreciate some help benchmarking the net impact on realistic-ish scenes. breakout, bevy_city and many_animated_foxes are probably a decent scattering, but I'd be very open to other suggestions.

Having done this refactor, I think that it's a net improvement for robustness and clarity even without the perf benefits however, and that we should proceed unless this is a clear regression.

alice-i-cecile · 2026-03-04T22:50:46Z

            entities.shuffle(&mut deterministic_rand());
            let mut query = SystemState::<Query<&Table>>::new(&mut world);
-            let query = query.get(&world);
+            let query = query.get(&world).unwrap();


TBH this is a pretty weird pattern. Who uses SystemState to query on the World?

TBH this is a pretty weird pattern. Who uses SystemState to query on the World?

It's silly when there is only one Query since they could just have used QueryState, but using SystemState<(Query<A>, Query<B>)> to split the world seems reasonable. And it's going to feel bad to have to unwrap() there even though Query parameters never fail.

I think we're eventually going to want an InfallibleSystemParam subtrait like

trait InfallibleSystemParam: SystemParam { fn get_param(/* same params as `SystemParam::get_param` */) -> Self::Item<'w, 's>; }

And then have fallible and infallible versions of SystemState::get.

That doesn't need to be in this PR, of course, although it will feel a little silly to add all these unwrap()s if we then get to remove them again soon.

I think that we should instead be pushing users to just call World::run_system more for these sorts of world-splitting use cases. The ergonomics are ultimately always going to be much better, and it's more consistent with other usages.

Then: no more internal complexity!

I think that we should instead be pushing users to just call World::run_system more for these sorts of world-splitting use cases. The ergonomics are ultimately always going to be much better, and it's more consistent with other usages.

Then: no more internal complexity!

Yeah, run_system_cached has really nice ergonomics when it works! I don't think it's a complete replacement for SystemState yet, though. It's a little less flexible, since needing a function prevents certain kinds of control flow, and it's a little less efficient, since you need to look up the system state in the world.

Mhmm, but for tests and benchmarks it's a much better pattern! It just didn't exist when these were written.

alice-i-cecile · 2026-03-04T22:52:14Z

 }
+
+#[cfg(test)]
+mod validation_tests {


I split this into its own test module for clarity. This seemed like a sensible enough home, but all these tests are related to each other but not really the other tests in this file.

alice-i-cecile · 2026-03-04T22:53:17Z


        should_run &= system_conditions_met;

-        if should_run {


Here's where we're removing the pre-thread spawning validation check.

alice-i-cecile · 2026-03-04T22:54:50Z

    ///
    /// [`ExclusiveSystemParamFunction`]: super::ExclusiveSystemParamFunction
-    fn get_param<'s>(state: &'s mut Self::State, system_meta: &SystemMeta) -> Self::Item<'s>;
+    fn get_param<'s>(


Exclusive system params were currently infallible, but that's a silly inconsistency. I took the liberty of fixing that up here too.

alice-i-cecile · 2026-03-04T22:56:00Z

-    /// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data
-    ///   registered in the access returned from [`System::initialize`]. There must be no conflicting
-    ///   simultaneous accesses while the system is running.
-    unsafe fn validate_param_unsafe(


Two whole methods gone for every system! This might actually improve binary size slightly.

alice-i-cecile · 2026-03-04T22:57:48Z

        world: UnsafeWorldCell<'world>,
        change_tick: Tick,
-    ) -> Self::Item<'world, 'state>;
+    ) -> Result<Self::Item<'world, 'state>, SystemParamValidationError>;


This is the core change of the whole PR: everything else flows from this signature change.

alice-i-cecile · 2026-03-04T23:01:00Z

    );

-    /// Refer to [`SystemParam::validate_param`].
+    /// Validates the inner parameter by calling [`SystemParam::get_param`] and discarding the value.


I wasn't confident enough on the usage of DynParamState to make serious changes here. Guidance welcome!

TBH, I kind of think we should just remove this. This is the same decoupled check-then-use pattern that's really problematic. Not sure how though.

I wasn't confident enough on the usage of DynParamState to make serious changes here. Guidance welcome!

Let's try to keep it consistent with ParamSet. The problem with both DynSystemParam and ParamSet is that they evaluate the inner parameter(s) on demand instead of up front, so we should be able to solve both the same way.

The approach you have here of validating the inner parameter during DynSystemParam::get_param seems great! It preserves the existing behavior, and the cost of calling get_param twice is no more than we paid before this PR.

But if we decide to make ParamSet return Result or have an InfallibleSystemParam bound, then we should make the same change here.

That's good advice, thank you. I think that I want to change ParamSet to return a Result: it's low enough usage that it's not terrible to do, and frankly I think it's a bit sloppy to have unavoidable panics like that when we could simply not.

I've decided against returning a Result here, and I'm doing early validation of ParamSet in 69cb5c0.

This is now consistent, so I think that's the best we can do in this PR.

alice-i-cecile · 2026-03-04T23:01:15Z

    /// # Safety
-    /// Refer to [`SystemParam::validate_param`].
-    unsafe fn validate_param(
+    /// Refer to [`SystemParam::get_param`].


Bad safety comment, but not any worse than before 🙃

kfc35

I support the merging of validate_param and get_param, so I’m happy to see this being pursued.

With the caveat of my inability to fully understand more complex rust macros at this point in my journey and my potential unawareness of the subtleties between all the permutations of system parameters, I approve of this. I didn’t notice any inconsistencies in any of the merges that would’ve prompted me to ask a question.

I’ll also add that I did some profiling/benchmarking for the first time using this branch vs main and also vs main before resources, if that helps. I used bevymark cargo run --release --features bevy/trace_tracy --example bevymark -- --waves 60 --per-wave 500 --benchmark --mode mesh2d since that’s what François used. GitHub won’t let me upload my screenshots of tracy ugh but in short:

For frame time (just purely looking at the frame compare mode): main currently vs main before resources as entities - current main has a mean frame time that is 3.35ms more than main before resources as entities
this branch vs main before resources as entities - this branch has a mean time frame that is only 721.7 μs more than main before resources as entities. So, an improvement of 2.63ms in mean frame time off of the original 3.35ms on my machine. (The median time incidentally increased by 18μs between this branch and current main but I assume that’s not that significant?)

edit: I forgot to mention that i also traced using bevycity between current main and this branch and there was an 8.55 ms improvement in mean frame time. The median time increased by 91μs. Bevy city was added 5 days after resources as entities so I wasn’t able to test “main before that change"

kfc35 · 2026-03-07T03:42:21Z

        }
    }

    /// Calls a closure for each parameter in the set.


Suggested change

/// Calls a closure for each parameter in the set.

///

/// # Panics

///

/// If system parameter validation fails for any parameter, a panic will occur.

I agree that this should be consistent between get_mut and for_each, but as the validation makes this nearly impossible, I'd be inclined to leave it out of both.

If we do want to mention panics here, we should probably also mention them in the macro-generated pN() methods for ordinary tuple ParamSets.

chescock · 2026-03-07T16:44:43Z

        }
    }

    /// Calls a closure for each parameter in the set.


I agree that this should be consistent between get_mut and for_each, but as the validation makes this nearly impossible, I'd be inclined to leave it out of both.

If we do want to mention panics here, we should probably also mention them in the macro-generated pN() methods for ordinary tuple ParamSets.

Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

Co-authored-by: Kevin Chen <chen.kevin.f@gmail.com>

alice-i-cecile added 9 commits March 4, 2026 10:20

Describe strategy in the migration guide

9d3a8ed

Note SystemState changes that will be needed

b92e3d9

Initial migration

1b739a6

Beef up migration guide to account for changes required

7c0c045

Add tests for validation occurring when systems are run

8303d4f

Perform the same changes for ExclusiveSystemParam

fbcbbb9

Cargo fmt

026c974

Note panic in ParamSet::get_mut

fbff8d2

Improve migration guide for Multithreaded executor

e604357

alice-i-cecile requested review from MiniaczQ and chescock March 4, 2026 22:49

github-project-automation Bot added this to ECS Mar 4, 2026

github-project-automation Bot moved this to Needs SME Triage in ECS Mar 4, 2026

alice-i-cecile commented Mar 4, 2026

View reviewed changes

Comment thread crates/bevy_ecs/src/system/system_param.rs

alice-i-cecile commented Mar 4, 2026

View reviewed changes

h2 headers in migration guide

0a280d6

chescock reviewed Mar 5, 2026

View reviewed changes

Comment thread examples/games/breakout.rs

Don't format twice for no reason

d151037

alice-i-cecile force-pushed the param-validation-merging branch from 30a25cd to d151037 Compare March 6, 2026 01:28

Validate ParamSet sub-params eagerly, and skip if any fail

69cb5c0

alice-i-cecile mentioned this pull request Mar 6, 2026

Remove unnecessary usage of SystemState API in tests and benchmarks #23238

Closed

kfc35 self-requested a review March 7, 2026 02:20

kfc35 approved these changes Mar 7, 2026

View reviewed changes

chescock approved these changes Mar 7, 2026

View reviewed changes

Remove panics comment

4958e80

Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

alice-i-cecile moved this from Needs SME Triage to SME Triaged in ECS Mar 9, 2026

alice-i-cecile added 3 commits March 9, 2026 14:37

Merge branch 'main' into param-validation-merging

3f700e1

Fix failing test by making sure error message does not change

90ac6a1

Update docs on get_param to match chescock's suggestion

5c5674f

alice-i-cecile added this pull request to the merge queue Mar 10, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 10, 2026

kfc35 reviewed Mar 11, 2026

View reviewed changes

Comment thread crates/bevy_winit/src/state.rs Outdated

Fix missing unwrap...

34fc43b

Co-authored-by: Kevin Chen <chen.kevin.f@gmail.com>

alice-i-cecile enabled auto-merge March 11, 2026 18:33

alice-i-cecile added this pull request to the merge queue Mar 11, 2026

Merged via the queue into bevyengine:main with commit b265dc0 Mar 11, 2026
38 checks passed

github-project-automation Bot moved this from SME Triaged to Done in ECS Mar 11, 2026

chescock mentioned this pull request Mar 12, 2026

InfallibleSystemParam #23339

Closed

hymm mentioned this pull request Mar 13, 2026

Performance regression since resource as components #23039

Open

urben1680 mentioned this pull request Mar 31, 2026

The safety invariant of SystemParam::validate_param is not documented. #16651

Open

ChristopherBiscardi added a commit to ChristopherBiscardi/bevy_enhanced_input that referenced this pull request Apr 4, 2026

SystemParam get returns a result as of: bevyengine/bevy#23225

c53243b

ChristopherBiscardi mentioned this pull request Apr 4, 2026

0.19 main simgine/bevy_enhanced_input#318

Merged

+    /// Calls a closure for each parameter in the set.
+    ///
+    /// # Panics
+    ///
+    /// If system parameter validation fails for any parameter, a panic will occur.

Uh oh!

Conversation

alice-i-cecile commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Performance discussion

Paths not taken

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfc35 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alice-i-cecile commented Mar 4, 2026 •

edited

Loading

kfc35 left a comment •

edited

Loading