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

Make tokio / async-io optional task execution engines #2624

Closed
wants to merge 4 commits into from

Conversation

hanabi1224
Copy link

@hanabi1224 hanabi1224 commented Aug 9, 2021

Make tokio / async-io optional task execution engines behind optional features

Objective

Solution

  • Leverage crate async-global-executor which uses compatible Task framework (async-executor) and has capabilities to switch to other task execution engines.
  • Code change to bevy is very minimal
  • It only adds one thin shim crate to the dependency tree on default feature set.
  • This also re-exports on_block method from bevy_tasks crate under bevy::tasks namespace, so that users don't need to depend on a task engine explicitly.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 9, 2021
@alice-i-cecile alice-i-cecile added A-Tasks Tools for parallel and async work C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Aug 16, 2021
@elpiel
Copy link

elpiel commented Jan 29, 2022

I'd love to have this option! The tonic default transport uses tokio and is unusable at the moment.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This is a compelling little opt-in feature, and the design is very clean. I like the block_on re-export a lot.

The changes here are very scoped.

@alice-i-cecile
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Feb 25, 2022

try

Merge conflict.

@alice-i-cecile
Copy link
Member

@hanabi1224 Sorry that this has been left in limbo for so long. If you're around, could you rebase this?

If we don't hear back by the time this is ready to merge, we'll do so ourselves (and credit you as the author regardless).

@hymm
Copy link
Contributor

hymm commented Feb 25, 2022

You can run tokio futures on bevy's executor today with https://docs.rs/async-compat/latest/async_compat/. What's the advantage of this approach over that?

@hanabi1224
Copy link
Author

@alice-i-cecile No problem, I just resolved all conflicts and have all CI jobs passing

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.

This seems like a great change for those looking for tokio support or in general better async task performance.

However, I don't see any initialization code at all or anything that enqueues tasks added to TaskPools to the global executors. If the intent is just to allow global executors to run alongside the Bevy TaskPools, this change is sufficient, though I'm concerned with how we're adding yet more threads to the engine by default, but if we want to leverage them for running the core of bevy itself, there will need to be changes with how TaskPool is structured.

@@ -36,4 +36,6 @@
|wayland|Enable this to use Wayland display server protocol other than X11.|
|subpixel_glyph_atlas|Enable this to cache glyphs using subpixel accuracy. This increases texture memory usage as each position requires a separate sprite in the glyph atlas, but provide more accurate character spacing.|
|bevy_ci_testing|Used for running examples in CI.|
|tokio|Use tokio as task execution engine.|
Copy link
Member

Choose a reason for hiding this comment

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

Are these two features mutually exclusive? If they are, this docs should cover that mutual exclusion.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
@Pessi94
Copy link

Pessi94 commented Sep 7, 2022

Hello, is this feature still being considered? It would be nice to be able to use tokio with bevy

@alice-i-cecile
Copy link
Member

Yes, it's being considered, but not something that's particularly pressing. @mockersf, how do you feel about this? Would you vote to start the bypass timer? I'm not immediately sure I would, but I want your opinion.

@mockersf
Copy link
Member

mockersf commented Sep 7, 2022

The PR looks mostly good but it could use some cleanups and conflict resolution before doing that.

@hanabi1224 are you still around? Sorry for the long delay, could you update the PR? Otherwise we can mark it as available for adoption

@alice-i-cecile
Copy link
Member

Closed in favor of #6137.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tasks Tools for parallel and async work C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling .await inside async in AsyncComputeTaskPool.spawn() causes tokio panic
8 participants