Skip to content

Remove SimpleExecutor #18741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove SimpleExecutor #18741

wants to merge 4 commits into from
+18 −243

Conversation

ElliottjPierce
Copy link
Contributor

@ElliottjPierce ElliottjPierce commented Apr 6, 2025 •
edited
Loading

Objective

Reduce complexity and code duplication of schedule executors. This is an alternative to fix #18453.

It sounds like this was a temporary solution to "sync points" in schedules, but now those can be inferred, so SimpleExecutor is unnecessary now.

Further, SimpleExecutor and SingleThreadedExecutor were very similar, which was becoming a consistency and code quality headache.

Solution

Remove SimpleExecutor.

Testing

CI

@ElliottjPierce ElliottjPierce added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 6, 2025

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@ElliottjPierce ElliottjPierce added the D-Trivial Nice and easy! A great choice to get started with Bevy label Apr 6, 2025
SpecificProtagonist
SpecificProtagonist approved these changes Apr 6, 2025
View reviewed changes
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, we don't need both this and SingleThreadedExecutor anymore, and the latter is only slightly more complicated and more consistent with MultithreadedExecutor.

Migration guide is fine and no other traces remain.

@ElliottjPierce ElliottjPierce mentioned this pull request Apr 6, 2025
Open
chescock
chescock approved these changes Apr 7, 2025
View reviewed changes
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, less code!

release-content/migration-guides/removed_simple_executor.md Outdated

Bevy has removed `SimpleExecutor`, one of the `SystemExecutor`s in Bevy alongside `SingleThreadedExecutor` and `MultiThreadedExecutor` (which aren't going anywhere any time soon).
The `MultiThreadedExecutor` is great at large schedules and async heavy work, and the `SingleThreadedExecutor` is good at smaller schedules or schedules that have fewer parallelizable systems.
So what was `SimpleExecutor` good at? Not much. That's why it was removed. If you're curious, it was originally created to function without any schedule "sync points", which apply deferred commands.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth being a little more precise here, in case anyone is relying on the exact behavior. The difference between SimpleExcecutor and SingleThreadedExcecutor is that SimpleExecutor applies deferred commands after every system, while SingleThreadedExecutor only applies them at sync points.

So, changing to SingleThreadedExecutor will usually work the same, but users might need to add sync points, especially if they had been using methods like after_ignore_deferred. Worse, I think that if you add no schedule constraints at all, then SimpleExecutor would happen to run the systems in the order they were inserted, but we wouldn't automatically add sync points. So it might be necessary to add before/after/chain calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I've updated the migration guide, but feel free to request changes if you see anything to improve!

@ElliottjPierce ElliottjPierce mentioned this pull request Apr 7, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2025
@ElliottjPierce @alice-i-cecile @sirius94
deprecate SimpleExecutor (#18753)
af8d12c 
# Objective

Contributes to #18741 and #18453.

## Solution

Deprecate `SimpleExecutor`. If users run into migration issues, we can
backtrack. Otherwise, we follow this up with #18741

We can't easily deprecate the module too because of
[this](rust-lang/rust#47238).

## Testing

CI

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Cyrill Schenkel <cyrill.schenkel@gmail.com>
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
@ElliottjPierce @alice-i-cecile
@sirius94 @andrewzhurov
deprecate SimpleExecutor (bevyengine#18753)
09fdee9 
# Objective

Contributes to bevyengine#18741 and bevyengine#18453.

## Solution

Deprecate `SimpleExecutor`. If users run into migration issues, we can
backtrack. Otherwise, we follow this up with bevyengine#18741

We can't easily deprecate the module too because of
[this](rust-lang/rust#47238).

## Testing

CI

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Cyrill Schenkel <cyrill.schenkel@gmail.com>
@Trashtalk217 Trashtalk217 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jun 16, 2025
janhohenheim
janhohenheim suggested changes Jun 16, 2025
View reviewed changes
release-content/migration-guides/removed_simple_executor.md
@@ -0,0 +1,15 @@
---
title: Removed Simple Executor
Copy link
Member

@janhohenheim janhohenheim Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration guide seems to be duplicate now, see simple_executor_going_away.md, which is about the deprecation of the executor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should be one or the other.

Do we want to remove it for 0.17 or just deprecate? If just deprecate, we should save this for 0.18. Otherwise, I'll update this pr and remove the old migration guide.

Copy link
Member

@janhohenheim janhohenheim Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecating feels like a nicer migration experience :)

That way users can ask for help while upgrading without their app being broken until they made the switch to another executor

Copy link
Member

@janhohenheim janhohenheim Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alice-i-cecile could we get a 0.18 milestone? Or do we just not merge this until 0.17 is released?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll let this go cold and we can come back to it later.

@alice-i-cecile alice-i-cecile added this to the 0.18 milestone Jun 16, 2025
@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janhohenheim janhohenheim janhohenheim requested changes

@chescock chescock chescock approved these changes

@Trashtalk217 Trashtalk217 Trashtalk217 approved these changes

@SpecificProtagonist SpecificProtagonist SpecificProtagonist approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events D-Trivial Nice and easy! A great choice to get started with Bevy S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
0.18
Development

Successfully merging this pull request may close these issues.

Increase structure for schedule executors to reduce boilerplate and ensure consistency
6 participants
@ElliottjPierce @chescock @janhohenheim @Trashtalk217 @SpecificProtagonist @alice-i-cecile