-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Allow using async_io::block_on in bevy_tasks #9626
Conversation
Question: should this feature be transitively exposed by other Bevy crates that depend on |
How did you produce that performance graph? I would like to be able to measure it for myself, and I have an idea for a minor optimization that I want to test. |
I used Tracy https://github.com/bevyengine/bevy/blob/main/docs/profiling.md#tracy-profiler. There a windows binary, but if you're on other platforms you'll have to compile it. I usually run with |
There's no reason why that shouldn't work, as it's just an ordinary feature flag. I think the feature should at least be exposed through the root |
I've profiled my idea and it does reclaim almost all of the lost performance, however it's a patch to async-io, not to Bevy, so we'll have to accept the reduced performance unless async-io releases a new version with the patch. |
79ee290
to
2f67624
Compare
Am I understanding correctly that enabling this is running the async-io futures on the thread that the block_on is running on? That makes me a bit nervous as that thread is the main thread in bevy and could potentially lead to more jitter in frame times as the futures are no longer running in parallel. Also for this feature to work right it's dependent on bevy calling block_on. Neither of these are deal breakers necessarily, but definitely a little more awkward than I like. I suspect just running another async executor in another thread would be better in most situations. We recommend this today for users asking to use tokio. Having said that, if all you're running is io-like tasks on the main thread the impact is probably minimal. Do the bevy_tasks tests pass with the feature on? There is also a prior pr that also adds tokio block_on support #6137. When I was reviewing that one I didn't understand that the tasks were being run in the block_on. If we decide to merge this, we should probably do the same for tokio. |
I don't entirely understand what you're saying.
This PR changes all the places that Bevy already calls
From what I understand, running multiple async executors in one application is a bad idea. This PR exists specifically so that can be avoided for people who want to use async-io with Bevy, given that
That PR seems to do a similar thing to this PR, but in a more complicated and roundabout way, by going through async-global-executor instead of just choosing the |
# Objective Fixes bevyengine#9625 ## Solution Adds `async-io` as an optional dependency of `bevy_tasks`. When enabled, this causes calls to `futures_lite::future::block_on` to be replaced with calls to `async_io::block_on`. --- ## Changelog - Added a new `async-io` feature to `bevy_tasks`. When enabled, this causes `bevy_tasks` to use `async-io`'s implemention of `block_on` instead of `futures-lite`'s implementation. You should enable this if you use `async-io` in your application.
Objective
Fixes #9625
Solution
Adds
async-io
as an optional dependency ofbevy_tasks
. When enabled, this causes calls tofutures_lite::future::block_on
to be replaced with calls toasync_io::block_on
.Changelog
async-io
feature tobevy_tasks
. When enabled, this causesbevy_tasks
to useasync-io
's implemention ofblock_on
instead offutures-lite
's implementation. You should enable this if you useasync-io
in your application.