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

PoC wasm thread implementation #631

Closed
wants to merge 2 commits into from

Conversation

chemicstry
Copy link

@chemicstry chemicstry commented Oct 5, 2020

Opening this as a draft example of implementing wasm threads. This is implemented with the async way as discussed on discord.

wasm thread implementation is provided by a wasm_thread that I created for this purpose. It abstracts some annoying cases about web workers, namely it does not require bundling additional worker script and can figure out wasm-bindgen shim script URL automatically so it acts as a normal std::thread. The main thread blocking issues are still there though.

The headless_wasm example can be built with build_wasm.sh script. Serve examples/wasm over HTTP and open headless_wasm.html. This requires nightly rust, because wasm atomics are not yet stable.

There is also an issue with getrandom which is currently patched in Cargo.toml.

@chemicstry chemicstry marked this pull request as draft October 5, 2020 21:33
request_handler.handle_request(&load_request).await;
})
.detach();
// self.task_pool
Copy link
Author

Choose a reason for hiding this comment

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

This for some reason fails with:

error: future cannot be sent between threads safely
   --> crates\bevy_asset\src\asset_server.rs:283:22
    |
283 |                     .spawn(async move {
    |                      ^^^^^ future created by async block is not `Send`
    |
    = help: the trait `Send` is not implemented for `dyn std::future::Future<Output = ()>`
note: future is not `Send` as it awaits another future which is not `Send`
   --> crates\bevy_asset\src\asset_server.rs:284:25
    |
284 |                         request_handler.handle_request(&load_request).await;
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ await occurs here on type `Pin<Box<dyn std::future::Future<Output = ()>>>`, which is not `Send`

@@ -106,7 +111,8 @@ impl TaskPool {

let executor = Arc::new(async_executor::Executor::new());

let num_threads = num_threads.unwrap_or_else(num_cpus::get);
//let num_threads = num_threads.unwrap_or_else(num_cpus::get);
Copy link
Author

Choose a reason for hiding this comment

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

TODO: num_cpus does not support wasm (just returns 1). Submit a PR implementing https://rustwasm.github.io/wasm-bindgen/api/web_sys/struct.Navigator.html#method.hardware_concurrency

Copy link
Member

@smokku smokku Oct 6, 2020

Choose a reason for hiding this comment

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

let num_threads = web_sys::window().unwrap().navigator().hardware_concurrency() should work. Just needs web-sys with features = [ "Window", "Navigator" ].

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the snippet. Although what I wanted to say is that it would be better to implement this directly into num_cpus crate, which aims to be multi-platform.

@memoryruins memoryruins added the O-Web Specific to web (WASM) builds label Oct 6, 2020
@lwansbrough
Copy link
Contributor

The main thread blocking issues are still there though.

That issue -- for posterity's sake -- if I recall correctly, is that there is still a synchronous wait in the main thread which waits for the async systems to complete for each frame. Is that correct?

And then the solutions are to offload rendering to a wasm thread with an offscreen canvas, or use asynchronous atomics... right?

@chemicstry
Copy link
Author

chemicstry commented Oct 8, 2020

The main thread blocking issues are still there though.

That issue -- for posterity's sake -- if I recall correctly, is that there is still a synchronous wait in the main thread which waits for the async systems to complete for each frame. Is that correct?

That comment was more about wasm_thread library that it does not cover it and it is up to the application to ensure main thread is not blocked.

This PR, though, ensures that. Well, at least at the basic level. The whole bunch of async changes are there so that ECS executor is async and does not block. This allows the bare-bones bevy to run on multithreaded wasm without any issues.

bevy_wasm_threads

However, there are still issues that some ECS systems use futures::block_on, TaskPool::scope or any of the sync primitives.

Synchronous blocking (block_on, scope) is not necessarily bad, but it can cause deadlocks that are hard to track down. If main thread blocks on a task that uses proxied web API, then it will deadlock because main browser thread cannot serve proxied API call. Proxied API includes things like HTTP fetches, spawning workers. So if there is an ECS system that synchronously waits for an asset to load and it so happens that it is executed on the main thread, you will get a deadlock.

Sync primitives (RwLock, Mutex, CondVar, etc) use atomic instructions and instead of a deadlock will panic on the main thread if lock is contested. Although, I might not be 100% correct on this and some primitives may use spinlock instead of panic. Need to investigate this. Async atomics are an interesting idea, but this means that mutexes are also async and would propagate throughout all codebase. I don't think that this is an option.

Now the solutions I see to this:

  1. Don't do anything about it architecturally and ensure that blocking API is never used. This would require ECS systems to be async and TaskPool::scope_async is used instead of TaskPool::scope. @cart mentioned that he forsees a lot of usage of TaskPool::scope and this may turn out to be quite inconvenient.
  2. Exclude main thread from the TaskPool execution so that all "normal" systems can use blocking API without problems. Thread local systems would have to follow rules as above.
  3. Run entire bevy in web workers. This would be ideal and wouldn't even require async changes present in this PR. However, this requires major architectural changes:
    • Refactor bevy so that winit could be initialized on the main thread, while App is launched on a different thread. This would require some channel glue for events and forwarding any DOM changes like window resize, fullscreen toggle. This probably involves touching a lot of winit internals.
    • Ensure that wgpu supports OffscreenCanvas
    • Users would likely want to use some DOM manipulation in their systems, so there must be a way to forward this seamlessly.
    • Other things like web audio are not yet supported in web workers IIRC and would require glue

I see the third option as a most viable long term solution, however, it requires a lot of upfront work and is quite invasive architecturaly. Second option could work now without much effort, but I'm affraid that deadlocking issues could become quite annoying.

And then the solutions are to offload rendering to a wasm thread with an offscreen canvas, or use asynchronous atomics... right?

Probably answered this above

@memoryruins memoryruins mentioned this pull request Nov 8, 2020
Base automatically changed from master to main February 19, 2021 20:44
@chemicstry
Copy link
Author

Closing because it's outdated. Maybe someone from #88 will want to look at this if needed.

@chemicstry chemicstry closed this Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-Web Specific to web (WASM) builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants