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

wasm32 support #341

Closed
BartAdv opened this issue Feb 3, 2018 · 25 comments
Closed

wasm32 support #341

BartAdv opened this issue Feb 3, 2018 · 25 comments
Assignees
Milestone

Comments

@BartAdv
Copy link

BartAdv commented Feb 3, 2018

Hello,

Tried to build it with --target wasm32-unknown-unknown, haven't succeeeded. At first, I found out the dispatcher might be problematic (it's supporting parallel execution), so I went and annotated shred with conditional compilation directives (it already had it for emscripted, so I just duplicated it with analogue for target_arch = "wasm32" here. It builds fine then.

Going back to specs, I've overriden the shred to use my branch:

[dependencies.shred]
git = "https://github.com/BartAdv/shred"
rev = "4d6b1a9c4d0bc1cd8d7bba275e7fbb4d3be69f1b"

only to see

error[E0433]: failed to resolve. Use of undeclared type or module `imp`
  --> /home/bart/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.3.20/src/os.rs:35:18
   |
35 | pub struct OsRng(imp::OsRng);
   |                  ^^^ Use of undeclared type or module `imp`

error[E0433]: failed to resolve. Use of undeclared type or module `imp`
  --> /home/bart/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.3.20/src/os.rs:40:9
   |
40 |         imp::OsRng::new().map(OsRng)
   |         ^^^ Use of undeclared type or module `imp`

OK, I get that - rand crate relies on some sys stuff. So I wanted to fix whatever dep is wanting rand. Found out, the rayon-core uses it. As it turns out, rayon is used by hibitset (gated by feature flag parallel), so I wanted to check if I could gate the specs rayon dependncy similarily...:

diff --git a/Cargo.toml b/Cargo.toml
index ef2a182..a979465 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -23,10 +23,9 @@ derivative = "1"
 fnv = "1.0"
 hibitset = { version = "0.4.1", features = ["parallel"] }
 mopa = "0.2"
-shred = "0.5.0"
 shred-derive = "0.3"
 tuple_utils = "0.2"
-rayon = "0.8.2"
+rayon = { version = "0.8.2", optional = true}
 
 futures = { version = "0.1", optional = true }
 serde = { version = "1.0", optional = true, features = ["serde_derive"] }
@@ -34,9 +33,14 @@ serde = { version = "1.0", optional = true, features = ["serde_derive"] }
 # enable rudy via --features rudy
 rudy = { version = "0.1", optional = true }
 
+[dependencies.shred]
+git = "https://github.com/BartAdv/shred"
+rev = "4d6b1a9c4d0bc1cd8d7bba275e7fbb4d3be69f1b"
+
 [features]
 common = ["futures"]
 nightly = []
+parallel = ["hibitset/parallel", "rayon"]
 
 [package.metadata.docs.rs]
 features = ["common", "serde"]

...to no avail, the build is still giving me

error[E0433]: failed to resolve. Use of undeclared type or module `imp`
  --> /home/bart/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.3.20/src/os.rs:35:18
   |
35 | pub struct OsRng(imp::OsRng);
   |                  ^^^ Use of undeclared type or module `imp`

Now - maybe someone more compoenent than me could try and check it? Is it planned to support wasm32-unknown-unknown target?

@BartAdv
Copy link
Author

BartAdv commented Feb 4, 2018

OK, I've learnt bit more about those things, so I am able to proceed with that - I'm wondering if I could clean up a bit how this multithreading support is conditioned. As I currently see, the shred detects whether we build for wasm32, and disables with that, whereas hibitset uses parallel feature flag. specs doesn't use it, so it could be added, just wondering what's better approach...

// EDIT

Ah, I'm having clearer picture now. the wasm32 detection (currently emscripten, but that can be easily changed) is for the async dispatcher, whereas parallel is for rayon stuff...

@BartAdv
Copy link
Author

BartAdv commented Feb 4, 2018

Got the WIP here: BartAdv@dd6052b

@BartAdv
Copy link
Author

BartAdv commented Feb 4, 2018

Since the discussion got a bit spread amongst repos/PRs, I want to step back a bit, and decide what would be best approach:

  1. extract parallel features from specs into specs-parallel?
  2. conditionally build with parallel support with parallel feature

On top of that: is it really only about parallel? shred depends on pulse, which is for 'async signals'. Can it be put under the same umbrella as parallel? When it comes to wasm32, you can't have thread, but I'm not sure about some async stuff...

BartAdv referenced this issue in BartAdv/specs Feb 4, 2018
to control inclusion of functionality requiring multithreading.
@torkleyy
Copy link
Member

torkleyy commented Feb 4, 2018

Yeah, let's just put it all under a parallel flag. I'm wondering, how long will it take for wasm to support multi-threading?

@torkleyy
Copy link
Member

@BartAdv Are you still interested in this?

@BartAdv
Copy link
Author

BartAdv commented Feb 17, 2018

@torkleyy yes! It slipped off my radar while waiting for shred changes to be merged. I think I got all the bits ready somewhere, because I'm already building a project using specs with wasm, so I'll just check what's needed to finalize it, most likely later this day.

@torkleyy
Copy link
Member

Great, thank you!

@BartAdv
Copy link
Author

BartAdv commented Feb 17, 2018

@torkleyy OK, so it seems to be all ready in this branch, but it references shred via git hash, so I guess first we need shred release?

@torkleyy
Copy link
Member

shred had a release lately, there's an open PR that bumps the shred version.

@BartAdv
Copy link
Author

BartAdv commented Feb 17, 2018

OK, ping me when it's done, I'll rebase on top of that then.

@torkleyy
Copy link
Member

@BartAdv It's merged ;)

@BartAdv
Copy link
Author

BartAdv commented Feb 21, 2018

Yep, sorry for delay, I've opened the PR #353 for that

@torkleyy torkleyy added this to the 1.0 milestone May 10, 2018
@richardanaya
Copy link

Any updates on this? :) I keep poking my head into this issue. It sounds like shred is wasm compliant now? What major pieces remain.

@BartAdv
Copy link
Author

BartAdv commented Sep 26, 2018

Unfortunately I'm not tracking it anymore (and not working on a project that needs this)...

@torkleyy
Copy link
Member

@richardanaya Yes, that's right, shred should be wasm compliant (at least I built it to be), although it's currently not tested for that.

Before I tell you what remains, I'd like to say: in theory, we could just take the current code, feature flag everything in Specs that uses rayon and be done with it. However, that's not the solution I had in mind and I'm not quite comfortable implementing it. I'd like to be able to compile any Specs code to wasm, not just code that was written specifically for it. There are two issues remaining for that to work:

  • ParJoin: that's an API in Specs that allows parallel iteration over (a group of) component storages; unfortunately it uses rayon's ParallelIterator trait and the only way of making it compatible would be writing our own wrapper API which is not very appealing
  • ThreadPool: several dependant crates are using rayon's thread pool to execute jobs. While we can emulate parallel, synchronous jobs (like parallel iteration), asynchronous jobs don't work.

That's why at this point we thought it would be easier to wait for threading support in wasm than trying to work around the problem. I don't know if there are any updates on that; should there be support by now it should be relatively easy to implement.

@torkleyy torkleyy added hard and removed easy labels Sep 26, 2018
@willglynn
Copy link
Contributor

Status links:

A related issue is SharedArrayBuffer browser support, which was yanked by everybody pending Spectre/Meltdown mitigations, and was just recently re-enabled in Chrome 68 due to some significant browser memory model changes. This work suggests a shorter path for getting wasm32 threads in Chrome, and a longer path for everyone else.

@bgourlie
Copy link

For what it's worth, I was able to get get the clusterbomb example compiled and working in the browser. At a high level I had to:

  • Pull in specs as a git dependency since this commit is not yet released
  • set default-features = false for specs in my Cargo.toml
  • Depend on rand with feature wasm-bindgen
  • Change durability_range.sample(..) to rng.gen_range(..) and get rid of any references to Range
  • Change all par_join to join
  • Create console_log! macro and use that instead of println!

@bgourlie
Copy link

bgourlie commented Oct 26, 2018

Wanted to give a quick update on this. There are still challenges unrelated to getting specs to compile to wasm that stand in the way of using specs for anything non-trivial in the browser. At a high-level, constraints enforced by wasm-bindgen and the nature of game loops in javascript are proving difficult to work around. I've outlined the challenges in this stackoverflow post.

Edit: You can see my current progress here: https://github.com/bgourlie/hexthing_wasm

@liamcurry
Copy link

Any progress/plans on this?

@torkleyy
Copy link
Member

@liamcurry This is mostly blocked on rayon; there are two options. We could either implement wasm support for rayon, or implement a fallback for wasm.

Specs should be usable with wasm (with some challanges noted by @bgourlie) and missing support for par_join (blocked on rayon).

This is not a must-have for 1.0.

@liamcurry
Copy link

I can confirm that the latest version of Specs works fine with WASM as long as you disable default features (the parallel feature flag specifically). Thanks @torkleyy!

@stale
Copy link

stale bot commented Mar 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Feel free to comment if this is still relevant.

@stale stale bot added the stale No activity since 60, issue is about to be closed label Mar 30, 2019
@torkleyy
Copy link
Member

This is still relevant.

@stale stale bot removed the stale No activity since 60, issue is about to be closed label Mar 31, 2019
@erlend-sh
Copy link
Member

erlend-sh commented Mar 31, 2019

Relevant rayon PR: rayon-rs/rayon#636

@torkleyy
Copy link
Member

Initial support has landed (non-parallel), #580 tracks further progress.

tuzz added a commit to tuzz/game-engine that referenced this issue Aug 22, 2019
WASM doesn’t support parallel execution so we need
to set default-features = false.

amethyst/specs#341 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants