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 2 #6137

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

Conversation

Pessi94
Copy link

@Pessi94 Pessi94 commented Oct 1, 2022

This is a copy of abandoned PR: #2624, I hope every original assumption is kept. All credit should go to @hanabi1224 as he/she is the author of 99% of the code present here and I just resolved conflicts so this could be reviewed again and possibly merged. Quick test shows that added features are not exclusive.

@Pessi94
Copy link
Author

Pessi94 commented Oct 11, 2022

Hello, I can see that both older and newer PRs are reviewed/commented, is there any particular reason for this PR not being reviewed? Can I do anything to speed it up?

@mockersf
Copy link
Member

This PR is harder to review. While the code change is small, reviewing it properly would mean cloning the PR locally, write an example with tokio compatibility enabled and tokio tasks, and looking at how it behaves, comparing to similar tasks without tokio

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.

My comments on the earlier PR still remain:

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.

@james7132 james7132 added C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR and removed C-Enhancement A new feature labels Oct 18, 2022
@Pessi94
Copy link
Author

Pessi94 commented Oct 21, 2022

My whole intent was being able to use tokio in bevy in a way that will not force me to use async_compat which is not as convenient to use and probably not maintained anymore. I didn't analyze impact of adding new threads, if you believe it will negatively impact bevy then feel free to close it this PR.

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Feb 26, 2024

Related: #11995
For tokio, this means we can use tokio-rayon
async-io can probably follow the same convention

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 simple 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.

6 participants