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.

Sign up for GitHub

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

Jump to bottom

Make Tasks functional on WASM #13889

Merged
merged 15 commits into from
Jul 16, 2024
Merged

Make Tasks functional on WASM #13889

merged 15 commits into from
Jul 16, 2024

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jun 17, 2024 •
edited
Loading

Objective

Right not bevy's task pool abstraction is kind of useless on wasm, since it returns a FakeTask which can't be interacted with. This is only good for fire-and-forget it tasks, and isn't even that useful since it's just a thin wrapper around wasm-bindgen-futures::spawn_local

Solution

Add a simple Task<T> handler type to wasm targets that allow waiting for a task's output or periodically checking for its completion. This PR aims to give the wasm version of these tasks feature parity with the native, multi-threaded version of the task

Testing

  • Did you test these changes? Not yet

@janhohenheim janhohenheim added C-Feature A new feature, making something new possible O-Web Specific to web (WASM) builds A-Tasks Tools for parallel and async work X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Async Deals with asynchronous abstractions D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jun 17, 2024
@alice-i-cecile
Copy link
Member

@kettle11 can I get your review here please?

@JoJoJet
Copy link
Member Author

JoJoJet commented Jun 17, 2024 •
edited
Loading

I don't agree that this is contentious, it's just finishing an incomplete feature. The considerations here are in terms of implementation details, not direction IMO

@JoJoJet JoJoJet removed the X-Contentious There are nontrivial implications that should be thought through label Jun 17, 2024
@alice-i-cecile alice-i-cecile added the X-Uncontroversial This work is generally agreed upon label Jun 17, 2024
@kettle11
Copy link
Contributor

This conceptually looks fine to me, but @JoJoJet have you checked that this still runs with the atomics flag enabled?

This PR has instructions for how to build with that enabled: #12205

That PR enabled Bevy Wasm to compile with threading enabled and make use of other libraries to spawn threads, parallelize, etc. even if Bevy Wasm itself isn't yet threaded internally.

@JoJoJet
Copy link
Member Author

JoJoJet commented Jun 17, 2024

I haven't tested that functionality yet but I see no reason it would stop working. I'm hoping that with this PR, full multi-threading support will just be a drop-in addition that utilizes this same interface

@JoJoJet JoJoJet marked this pull request as draft June 22, 2024 23:54
@janhohenheim janhohenheim mentioned this pull request Jun 23, 2024
Closed
@JoJoJet JoJoJet marked this pull request as ready for review July 8, 2024 19:34
@JoJoJet JoJoJet requested a review from janhohenheim July 8, 2024 21:31
janhohenheim
janhohenheim reviewed Jul 8, 2024
View reviewed changes
Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Only some documentation nits. I don't feel qualified to comment on the correctness of the implementation.

crates/bevy_tasks/src/wasm_task.rs Outdated Show resolved Hide resolved
crates/bevy_tasks/src/wasm_task.rs Outdated Show resolved Hide resolved
crates/bevy_tasks/src/wasm_task.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Jul 14, 2024
mockersf
mockersf approved these changes Jul 15, 2024 •
edited
Loading
View reviewed changes
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 16, 2024
Merged via the queue into bevyengine:main with commit ee15be8 Jul 16, 2024
32 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Jul 16, 2024
Closed
@janhohenheim janhohenheim added the C-Needs-Release-Note Work that should be called out in the blog due to impact label Jul 16, 2024
@janhohenheim janhohenheim added this to the 0.15 milestone Jul 16, 2024
sgshea added a commit to sgshea/sandengine that referenced this pull request Aug 22, 2024
@sgshea
feat: Multithreading & chunk fix
e7229e9 
(Below copied from performance.md)

- Multithreading, fix dirty chunks
    - Fix for dirty chunks was to check if a chunk was empty or not before updating, this gave most performance gains (to around 35-45us while few updates are happening)
    - Multithreading implementation using [bevy_tasks](https://docs.rs/bevy_tasks/latest/bevy_tasks/index.html), specifically the `ComputeTaskPool`

- No sand: 35us mean
- Drawing lots of sand at one: still around 490us
- Some constant movement in a few chunks such as water/smoke: 270us

Multithreading should help most when the world and chunk size gets much bigger.

Currently multithreading is only implemented in one spot (simulating a chunk), but I want to use the taskpool for other things in the future such as:
- Creating the dirty rectangles of chunks that need updating after simulation while still in thread (will probably need to use a threadsafe hashmap and/or mutexes)
- Updating the render image after simulation while in the thread, currently the whole image is created each frame and is a slow point, but we could just update chunks that have changed
- Creating the colliders for each chunk, this should also not recreate colliders for chunks that haven't changed, collider generation is currently the slowest part of the simulation

> Note on multithreading on web (WASM)
> Currently bevy does not support multithreaded execution on WASM builds, and the `bevy_tasks` module does not support multithreading.
> However, `bevy_tasks` support for web [is merged and due for bevy 0.15 (next release)](bevyengine/bevy#13889)
> The project still works on WASM but this means performance is abit slower than native, optimizations from the dirty chunk system still help a lot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cBournhonesque cBournhonesque cBournhonesque left review comments

@janhohenheim janhohenheim janhohenheim left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
A-Tasks Tools for parallel and async work C-Feature A new feature, making something new possible C-Needs-Release-Note Work that should be called out in the blog due to impact D-Async Deals with asynchronous abstractions D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes O-Web Specific to web (WASM) builds S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
0.15
Development

Successfully merging this pull request may close these issues.
6 participants
@JoJoJet @alice-i-cecile @kettle11 @cBournhonesque @mockersf @janhohenheim