Skip to content
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.

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

Add World::run_system_set to run only the systems in a SystemSet #12726

Closed

Conversation

cBournhonesque
Copy link
Contributor

Objective

Fixes #12717
As explained in the issue, it would be convenient to be able to run only the systems that are inside a SystemSet. (for example for benchmarking, I would like to only run a specific SystemSet of my App.

Solution

  • The idea is to re-use the work started from system stepping, which made it possible to provide a FixedBitSet of systems that need to be fixed when running the schedule.
  • Add a separate function run_system_set which computes the bitset of systems to skip from the graph:
    • run a BFS on the graph to find the NodeId of every system that needs to be run
    • convert that FixedBitSet into a FixedBitSet of the index of every system that needs to be skipped in the final SystemSchedule
  • Added some unit tests to check that the code works as expected

TODO:

  • ideally I would have created a function run_with_skip that takes a closure Box<dyn Fn(&Schedule) -> FixedBitSet> as input that defines which systems we want to skip from the schedule, but I wasn't able to get it working (my closure wasn't dyn Fn apparently? I would appreciate help there
  • clean PR + doctests
  • maybe add more doctests on the schedule_graph code? it was a bit hard to follow what was going on there because that code is not very documented

Changelog

Added

  • A function World::run_system_set to run once the systems that are part of a given SystemSet

@cBournhonesque cBournhonesque added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Mar 26, 2024
@hymm
Copy link
Contributor

hymm commented Mar 26, 2024

I'm not a fan of removing the feature guard on Executor::run for a niche feature like this. The guard is there so that production builds don't pay the cost of system stepping.

@cBournhonesque
Copy link
Contributor Author

I'm not a fan of removing the feature guard on Executor::run for a niche feature like this. The guard is there so that production builds don't pay the cost of system stepping.

The cost being the Option check when running the executor? I can look into that

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

System stepping was noted to have a significant performance impact at the start of 0.13 due to being enabled by default. This PR reverts that. We also made notable improvements to the scheduler performance as well, so this is worth reevaluating. Can you please profile some of the stress tests in the repo (many_foxes, many_cubes, etc.) to help ensure this isn't a performance regression?

@james7132 james7132 added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Mar 29, 2024
@cBournhonesque
Copy link
Contributor Author

cBournhonesque commented Apr 2, 2024

I'm not sure if I did it correctly; I ran many_boxes on my laptop but I had some other apps opened, + sometimes I minized the example.

image Yellow is old, red is new; so the change seems to have an impact ?

However I see the opposite effect when focusing on Schedule::main
image

@cBournhonesque
Copy link
Contributor Author

@hymm @james7132 I added a change that removed any perf impact, at the cost of some code duplication. I don't know if that is better?

@cBournhonesque
Copy link
Contributor Author

Actually, this functionality run_system_set would probably only be used for tests, since 99% of non-test use cases use run.
In which case it might be possible to extend existing the system-stepping machinery to achieve this.
I will try to add Behaviour::OnlyRun and add helper functions for SystemSets in system-stepping to achieve the same objective.

@cBournhonesque cBournhonesque added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label May 2, 2024
@dmlary
Copy link
Contributor

dmlary commented May 11, 2024

For benchmarking, I did add the empty_schedule_run() benchmark to benches/benches/bevy_ecs/scheduling/schedule.rs when we fixed the largest performance hit from stepping enabled (#11959). That's the benchmark that someone else was running that exposed the issue.

@cBournhonesque
Copy link
Contributor Author

I've tried another implementation that re-uses the stepping machinery; it might be cleaner.
@dmlary I wanted to add you as reviewer but for some reason you don't appear on the list of reviewers in github: #13219

@alice-i-cecile
Copy link
Member

I like the competing PR better :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce an app::run_system_set() function
5 participants