bevy
bevy copied to clipboard
bevyengine
Expose SystemExecutor & open up ExecutorKind again
What problem does this solve or what need does it fill?
During the implementation of #7267 & #6587 (stageless ECS schedules), the old 0.9 API of pub fn set_executor(&mut self, executor: Box<dyn ParallelSystemExecutor>) was replaced by a closed enum in the form of ExecutorKind. I'd like to see a similar API exposed again to be able to implement custom executors - specifically a Rayon-based one.
What solution would you like?
I think it would be sensible to add a variant like ExecutorKind::Custom(Box<dyn SystemExecutor>).
What alternative(s) have you considered?
A better abstraction level could be to allow other implementations of ComputeTaskPool instead, but I'm not familiar enough with the internals to know if it's feasible.
Additional context
I wasn't happy with the behavior (#4161) or performance of async_executor when used as the executor, so I rolled out my own implementation that I've been using successfully with bevy_ecs` 0.6.0 - 0.9.1. As of 0.10.0, there doesn't seem to be a way to use a custom executor.
This is a bit of a tough one. There's so much logic in the scheduler now that replicating everything for it is now no simple task. This is why we chose not to expose a user provided executor when we implement the RFC.. This would also mean more solidly enforcing the safety invariants or exposing a safer API for internal data types, as the safety invariants on some unsafe trait implementations are predicated on the fact they are used only in limited locations that are known to be safe.
This is definitely still doable, though exposing it is going to be much more involved.
I'm also interested in moving away from async_executor, especially if we can come up with a solution that works to satisfy all of the requirements of the engine that has lower overhead.
Rayon, unfortunately, isn't a great fit, IMO. It lacks support for running async tasks and does not work on WASM targets. We can always enable it on non-WASM targets, but we always need to be able to support heterogeneous async/synchronous tasks in the task pools.
@james7132 I was looking at the code and the usage for SyncUnsafeCell is completely isolated to the `Multithreaded executor now. Relevant code here. Were there other invariants we needed to worry about? If not it might be ok to expose as suggested above.
More long term I would like to have a clean separation between validating the schedule, converting into the runnable form, and consuming the runnable form to run it. This would allow for either replacing the executor, but keeping the topological sort or replacing both and having the built version of the schedule be different from a topological sort. (i.e. potentially separating things between multithreaded and single threaded sections.)
On rayon, I think the maintainers are open to (adding async support)[https://github.com/rayon-rs/rayon/pull/679#issuecomment-528296292]. Though I haven't talked to anyone. It sounds it'd just be a lot of work to enable. I'm not sure that not having WASM support is a huge blocker, since we're already hacking around that async-executor doesn't have it either. Do any async executors have decent support for WASM yet?
If rayon support is ever considered as a first-class feature, I think it should also support the parallel iterators on queries. I don't know if that would be a big effort or not. async_executor could still be used for the Async task pool, at least until Rayon gets it as well?