Use new run_without_applying_deferred method in SingleThreadedExecutor
#18684
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chescock
added
A-ECS
alice-i-cecile
approved these changes
Apr 2, 2025
alice-i-cecile
added
the
D-Straightforward
|
Less unsafe is more good! |
kristoff3r
reviewed
Apr 2, 2025
| world: &mut World, | ||
| ) -> Result { | ||
| let result = system.run_without_applying_deferred((), world); | ||
| black_box(()); |
There was a problem hiding this comment.
This is the first time I've seen black_box used like this, and after some searching I still don't know why this would be beneficial. Can you explain or link me to somewhere this is explained?
There was a problem hiding this comment.
I'm not actually sure! I just copied it from the other methods.
... Okay, I think I found the source. It comes from the __rust_begin_short_backtrace method in std, which has a comment "prevent this frame from being tail-call optimised away".
I'm going to add that as a comment to the black_box calls here, even though it's not strictly related to this PR.
SpecificProtagonist
approved these changes
Apr 2, 2025
alice-i-cecile
added
S-Ready-For-Final-Review
alice-i-cecile
approved these changes
May 6, 2025
andrewzhurov
pushed a commit
to andrewzhurov/bevy
that referenced
this pull request
May 17, 2025
…utor` (bevyengine#18684) # Objective Simplify code in the `SingleThreadedExecutor` by removing a special case for exclusive systems. The `SingleThreadedExecutor` runs systems without immediately applying deferred buffers. That required calling `run_unsafe()` instead of `run()`, but that would `panic` for exclusive systems, so the code also needed a special case for those. Following bevyengine#18076 and bevyengine#18406, we have a `run_without_applying_deferred` method that has the exact behavior we want and works on exclusive systems. ## Solution Replace the code in `SingleThreadedExecutor` that runs systems with a single call to `run_without_applying_deferred()`. Also add this as a wrapper in the `__rust_begin_short_backtrace` module to preserve the special behavior for backtraces.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Simplify code in the
SingleThreadedExecutorby removing a special case for exclusive systems.The
SingleThreadedExecutorruns systems without immediately applying deferred buffers. That required callingrun_unsafe()instead ofrun(), but that wouldpanicfor exclusive systems, so the code also needed a special case for those. Following #18076 and #18406, we have arun_without_applying_deferredmethod that has the exact behavior we want and works on exclusive systems.Solution
Replace the code in
SingleThreadedExecutorthat runs systems with a single call torun_without_applying_deferred(). Also add this as a wrapper in the__rust_begin_short_backtracemodule to preserve the special behavior for backtraces.