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 no_std support to bevy_tasks #15464

Merged
merged 13 commits into from
Dec 6, 2024

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Sep 27, 2024

Objective

Solution

  • Added the following features:
    • std (default)
    • async_executor (default)
    • edge_executor
    • critical-section
    • portable-atomic
  • Added edge-executor as a no_std alternative to async-executor.
  • Updated the single_threaded_task_pool to work in no_std environments by gating its reliance on thread_local.

Testing

  • Added to compile-check-no-std CI command

Notes

  • In previous iterations of this PR, a custom async-executor alternative was vendored in. This raised concerns around maintenance and testing. In this iteration, an existing version of that same vendoring is now used, but only in no_std contexts. For existing std contexts, the original async-executor is used.
  • Due to the way statics work, certain TaskPool operations have added restrictions around Send/Sync in no_std. This is because there isn't a straightforward way to create a thread-local in no_std. If these added constraints pose an issue we can revisit this at a later date.
  • If a user enables both the async_executor and edge_executor features, we will default to using async-executor. Since enabling async_executor requires std, we can safely assume we are in an std context and use the original library.

`thread_local`, `getrandom`, and `web-time` were being included even in `no_std` configurations. Additionally, `hashbrown` had extra features enabled that should've been gated.
@bushrat011899 bushrat011899 added C-Feature A new feature, making something new possible S-Blocked This cannot move forward until something else changes A-Tasks Tools for parallel and async work D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Async Deals with asynchronous abstractions labels Sep 27, 2024
@bushrat011899 bushrat011899 added the X-Controversial There is active debate or serious implications around merging this PR label Sep 27, 2024
@hymm
Copy link
Contributor

hymm commented Sep 27, 2024

Is it possible to try and upstream the no_std changes to async executor? I'm not sure bevy wants to pick which executor to use yet. See #6762 or #11995. If we do want to do this, we shouldn't just copy the code, but tests, benches, and CI too. (This would probably be a separate PR for ease of review.)

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

Awesome. Very clean.

@bushrat011899
Copy link
Contributor Author

Is it possible to try and upstream the no_std changes to async executor?

It might be possible! The reason I didn't start there is the crate describes itself as a reference implementation, and that you should write your own executor using the underlying async-tasks crate instead.

I'm not sure bevy wants to pick which executor to use yet. See #6762 or #11995. If we do want to do this, we shouldn't just copy the code, but tests, benches, and CI too. (This would probably be a separate PR for ease of review.)

Perfectly reasonable! I've copied over all the tests as well (this executor was only a single file), but I do agree that something like Tokio or Rayon could also work. But, I specifically just want to get no_std support over the line, and those crates aren't no_std.

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

Still good

@bushrat011899 bushrat011899 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Oct 9, 2024
@bushrat011899 bushrat011899 added this to the 0.16 milestone Oct 10, 2024
@hymm
Copy link
Contributor

hymm commented Nov 4, 2024

I've copied over all the tests as well (this executor was only a single file.

Tests are actually here: https://github.com/smol-rs/async-executor/tree/master/tests. The async-executor repo also runs loom on the tests. If this is merged we should figure out how to run loom on bevy_tasks.

But I'm really not sure if forking async executor is worth it just for no-std support. We're potentially locking bevy out from future upstream improvements. For consoles I imagine that they'll need to have their own fork of async executor anyways as they should be able to park and unpark threads, so would be able to have their own Mutex implementation, which would be better than spin locking. If no_std is only single threaded is there a different async executor we can use and then have a separate no_std task pool?

@bushrat011899 bushrat011899 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 4, 2024
@bushrat011899
Copy link
Contributor Author

I've copied over all the tests as well (this executor was only a single file.

Tests are actually here: https://github.com/smol-rs/async-executor/tree/master/tests. The async-executor repo also runs loom on the tests. If this is merged we should figure out how to run loom on bevy_tasks.

Thanks for pointing that out, not sure how I missed it! So used to having unit tests within the same file I got complacent.

But I'm really not sure if forking async executor is worth it just for no-std support. We're potentially locking bevy out from future upstream improvements. For consoles I imagine that they'll need to have their own fork of async executor anyways as they should be able to park and unpark threads, so would be able to have their own Mutex implementation, which would be better than spin locking. If no_std is only single threaded is there a different async executor we can use and then have a separate no_std task pool?

These are excellent points and I generally agree. There was some discussion in the Discord around potentially using embassy as a way to bring multi-threaded executor support anyway. I think I will update this PR to continue using async-executor for std targets (behind a default feature flag) and pivot the forked implementation to just be for no_std where embassy couldn't be used (very basic single-threaded only). As a follow-up PR, multi-threaded support could then be added via something like embassy (behind a non-default feature flag).

@bushrat011899
Copy link
Contributor Author

Haven't forgotten about this PR! I'm planning to redo this to instead use async-executor with std (the status quo), but switch to using edge-executor on no_std platforms. It's basically what I've done within this PR, but done externally with much better testing and a moderate user-base. I believe that should be sufficient to resolve the largest concerns with this PR.

@hymm
Copy link
Contributor

hymm commented Dec 3, 2024

That sounds like a good approach. Please ping/request review from me when you have that ready and I'll try to review it quickly.

@bushrat011899 bushrat011899 removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Dec 4, 2024
@bushrat011899 bushrat011899 added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Dec 4, 2024
@bushrat011899
Copy link
Contributor Author

I believe this PR is now at a point where it is ready for review! I've reset the branch back to main and rebuilt the functionality around edge-executor rather than vendoring an alternative to async-executor. If I have requested your review don't feel pressured to actually review the PR! I just wanted to clear any old approvals to avoid implying this was further along in its review process than it actually is.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

I'm pretty happy with this approach now. I only did a code review and the code changes seem reasonable. I haven't tried running this yet.

The one thing that is hard to understand is which combination of features we're supporting. Could we add some documentation in the bevy_tasks readme for what the different features are and how to use them?

We should probably manually test wasm32 and make sure it is working correctly. Did you try running this on a no_std platform?

crates/bevy_tasks/Cargo.toml Outdated Show resolved Hide resolved
tools/ci/src/commands/compile_check_no_std.rs Outdated Show resolved Hide resolved
Properly implement `portable-atomic` and `critical-section` features to enable compilation on `thumbv6m-none-eabi` (as an example platform)

Co-Authored-By: Mike <[email protected]>
@bushrat011899
Copy link
Contributor Author

The one thing that is hard to understand is which combination of features we're supporting. Could we add some documentation in the bevy_tasks readme for what the different features are and how to use them?

I've updated the README to list exactly what features you may need for no_std support. I am happy to add more documentation (including for the existing features) if there's desire for that too though!

We should probably manually test wasm32 and make sure it is working correctly. Did you try running this on a no_std platform?

I had previously tested this in QEMU for x86_64-unknown-none, but with the latest change I've also tested on a Raspberry Pi Pico, thumbv6m-none-eabi. I haven't done benchmarks to assess how fast it runs on those platforms, largely because it previously didn't work at all, so there's nothing to compare against.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Dec 6, 2024
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.

The technical strategy here looks solid, but I want more docs. This crate isn't far off from warn(missing_docs): let's not make it work. Simple doc strings for all of the new types and modules please? I think it will really help reviewers / contributors understand the purpose and structure of this.

@bushrat011899
Copy link
Contributor Author

I've updated the documentation for the entire crate, ensuring all public items have at least a simple docstring.

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.

Much better! These are good docs.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 6, 2024
Merged via the queue into bevyengine:main with commit 72f096c Dec 6, 2024
31 of 32 checks passed
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-Feature A new feature, making something new possible D-Async Deals with asynchronous abstractions D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants