Initial support for pthreads with wasm #5710

dschuff · 2017-10-25T17:19:20Z

Also adds support for testing with both wasm and JS+SAB, but there's no clean way to enable it yet.
Tested with EMSCRIPTEN_BROWSER='/path/to/chrome --user-data-dir=clean_dir --js-flags=--experimental-wasm-threads --enable-features=WebAssembly --disable-features=WebAssemblyTrapHandler' with a locally-built chrome but it should work with canary also.

dschuff · 2017-10-25T17:19:44Z

src/preamble.js

@@ -1839,7 +1849,7 @@ function addRunDependency(id) {
 #if USE_PTHREADS
  // We should never get here in pthreads (could no-op this out if called in pthreads, but that might indicate a bug in caller side,
  // so good to be very explicit)
-  assert(!ENVIRONMENT_IS_PTHREAD);
+  assert(!ENVIRONMENT_IS_PTHREAD || id === 'wasm-instantiate');


This is uuuuugly.

Did you make this change in order to get async instantiation working? It would probably be better if we avoided this altogether and follow the path "We can just use sync instantiation in the worker." that you wrote below?

Yeah I wrote it before I switched to sync instantiation, removed now.

Actually that's not quite right; preamble.js adds the dependency regardless of whether a custom instantiator is used. We can bypass that in threads (see 9d97628) which I guess is a little bit nicer than this. I also noticed that right now the user can't override wasm-instantiate in a pthread right now (only on the main thread). Is that something we need?

kripken · 2017-10-25T18:38:48Z

src/preamble.js

@@ -1237,6 +1238,15 @@ if (typeof Atomics === 'undefined') {
  Atomics['xor'] = function(t, i, v) { var w = t[i]; t[i] ^= v; return w; }
 }

+#else
+// TODO: Something smart about BINARYEN_MEM_MAX. Some default, also rename it.


is it right that wasm can't support memory growth with shared memory? if so, we can just show an error if growth or max > initial?

It can support growth, just not unconstrained growth. Or in other words, you can have max > initial, but you can't set an initial without a max.

I see, ok.

I still don't understand this code, though. Why do we create the memory here instead of later down where we create it normally (which also handles MEM_MAX)?

Basically just because of the overall structure of the ifdefs. All the pthreads stuff is first, and everything below is in the else (not pthreads). This whole nest of code is really ugly and complex because of all the options, should probably cleaned up and/or the options rethought.

Need to resolve this comment - I think we can make command line requirement to pass BINARYEN_MEM_MAX so that frontend makes sure in multithreaded mode this gets set?

OK, made it a requirement to add WASM_MEM_MAX when using wasm+pthreads and memory growth. I tried to add a test but it doesn't look like memory growth on a thread is working right now. Will address that separately.

kripken · 2017-10-25T18:39:56Z

src/preamble.js

@@ -2303,6 +2313,10 @@ function integrateWasmJS() {
      assert(Module === trueModule, 'the Module object should not be replaced during async compilation - perhaps the order of HTML elements is wrong?');
      trueModule = null;
 #endif
+#if USE_PTHREADS
+      // Keeep a reference to the compiled module so we can post it to the workers.
+      Module['wasmModule'] = output['module'];


the module should be saved as Module['asm'] already

The file preamble.js has Module['asm'] = exports;, so 'asm' seems to be getting the exports object, rather than the Module. So it looks like we do need to store this manually(?)

I think what we'd like is to make receiveInstance() take a second parameter that is the module, so that applications that instantiate themselves will also get the module stored for threads.

I do hate this a bit that we do have to store the module to be able to instantiate a new thread - that wastes memory; and I was pondering if we could reuse IndexedDB and the thread pool idea to optimize, i.e. if application knows they only need fixed K threads at startup, the could precreate a pool of that size to be able to throw away the Module afterwards. If new threads want to spawn after that, they would take a slow path of reading the Module from IndexedDB for instantiation. Although not sure if that's worth the trouble to optimize, so let's leave that for later.

kripken · 2017-10-25T18:41:50Z

tests/test_browser.py

@@ -3108,19 +3108,26 @@ def prep_no_SAB(self):
      </script>
    '''))

+  # Run a browser test both with and without wasm
+  def btest_wasm(self, *args, **kwargs):


for running as both asm and wasm, see the also_proxied flag to btest which makes us test both regular and proxied mode. maybe we can do the same pattern for asm and wasm.

I still don't see why we need a new method btest_wasm, can't be do something parallel to also_proxied, like also_wasm?

otherwise lgtm

Or is this different because it is off for now?

No, sorry I just hadn't gotten to that yet. Its switched now. I don't see the feature-detection mechanism you mentioned though. Since the tests run in a random order we can't just rely on the outcome of one of them to gate the rest. I thought I might just add a flag to runner.py for now.

BTW, chrome needs a flag to enable threads (which means modifying EMSCRIPTEN_BROWSER in the env) so I thought maybe just another env var for testing it since we have an env var anyway. Does nightly need a separate flag, or is it just on by default there?

Not sure about how we avoid pthreads tests if no pthreads support - @juj?

Also @juj would know about firefox nightly needing a flag for wasm threads or not as he tested it.

kripken · 2017-10-25T18:43:09Z

Need to think about the testing here. I think we have a mechanism to check if pthreads is supported and we skip those tests otherwise, maybe we need something similar for wasm-pthreads, since right now they aren't in firefox, so those tests would break.

kripken · 2017-11-01T16:51:33Z

There is now a firefox build with wasm threads support. I tested a few simple things and didn't see any issues. I did need to add

+    if shared.Settings.USE_PTHREADS:
+      cmd += ['--enable-threads']

around line 2263 of emcc.py though.

juj · 2017-11-02T14:02:36Z

Perfect, thanks @dschuff for getting this going!

I'm testing this against the Multithreading series of branches, which I'm looking to resume landing shortly. Currently running test suites on Firefox Nightly, so I'll get back with feedback on this in a moment.

juj · 2017-11-06T12:12:26Z

Test suites pass in FF Nightly, and also some code compiles; The tree https://github.com/juj/emscripten/commits/multithreading_5 is the current "latest state" for multithreading support that the Multithreading PRs are landing towards, see in particular juj@a1cf520 and juj@8690b9a that relate to this PR.

Unfortunately it looks like real world(tm) codebases don't still compile, I'm currently hunting some odd codegen related bug to understand this better. It could be related to the global data section initialization commit above, though not completely sure.

This reverts commit 8049139.

dschuff · 2017-11-16T01:22:11Z

OK, assuming it doesn't break any existing tests, this may be ready to land. Any more thoughts?

kripken · 2017-11-16T19:03:49Z

lgtm. the new wasm-pthreads are off by default, looking for that env var, correct?

@juj should look at this code too (before or after landing is ok i think).

juj · 2017-11-16T19:14:50Z

src/pthread-main.js

+        Module['wasmMemory'] = e.data.wasmMemory;
+        buffer = Module['wasmMemory'].buffer;
+      } else {
+        buffer = e.data.buffer;


This would be #if !WASM, or an #else block to above.

juj · 2017-11-16T19:15:39Z

src/shell.js

@@ -69,6 +69,8 @@ if (!ENVIRONMENT_IS_PTHREAD) ENVIRONMENT_IS_PTHREAD = false; // ENVIRONMENT_IS_P
 var PthreadWorkerInit; // Collects together variables that are needed at initialization time for the web workers that host pthreads.
 if (!ENVIRONMENT_IS_PTHREAD) PthreadWorkerInit = {};
 var currentScriptUrl = (typeof document !== 'undefined' && document.currentScript) ? document.currentScript.src : undefined;
+#else
+var ENVIRONMENT_IS_PTHREAD = false;


Let's not add this in at all, in singlethreaded builds we want to never see a variable ENVIRONMENT_IS_PTHREAD anyhwere. I like to keep this strict to enforce a "don't pay for what you don't use" barrier on this feature.

I agree in principle but here's where we have a tradeoff. Gating the 2 lines of ENVIRONMENT_IS_PTHREAD check above on USING_PTHREADS makes the code up there super ugly because of the way the ifdefs interact with conditionals in the code. I went ahead and did it, but I guess apropos of our discussion in #5732 it would be much much more readable to either (for example) turn ENVIRONMENT_IS_PTHREAD into a macro, or go all-in on closure and make everything regular JS code.

dschuff

should be all addressed except for improving handling of WASM_MEM_MAX (waiting for #5803)

dschuff · 2017-11-16T22:17:10Z

src/pthread-main.js

+  // We can just use sync instantiation in the worker.
+  instance = new WebAssembly.Instance(Module['wasmModule'], info);
+  // We don't need the module anymore; new threads will be spawned from the main thread.
+  // XXX is this true?


dschuff · 2017-11-16T22:22:25Z

src/pthread-main.js

+  Module['wasmModule'] = undefined;
+  receiveInstance(instance);
+  return instance.exports;
+}


Done. In asm.js this code will never be called and is harmless, so I suppose gating it on WASM just reduces the code size. Not sure how many people will be shipping anything real with asm.js threads before then? But here it's easy and clean.

dschuff · 2017-11-16T22:22:53Z

src/pthread-main.js

      Module['TOTAL_MEMORY'] = TOTAL_MEMORY = e.data.TOTAL_MEMORY;
      STATICTOP = e.data.STATICTOP;
      DYNAMIC_BASE = e.data.DYNAMIC_BASE;
      DYNAMICTOP_PTR = e.data.DYNAMICTOP_PTR;

+
+      if (e.data.wasmModule) {


good point, done.

dschuff · 2017-11-16T22:24:24Z

src/pthread-main.js

+        Module['wasmMemory'] = e.data.wasmMemory;
+        buffer = Module['wasmMemory'].buffer;
+      } else {
+        buffer = e.data.buffer;


dschuff · 2017-11-16T22:38:34Z

src/shell.js

@@ -69,6 +69,8 @@ if (!ENVIRONMENT_IS_PTHREAD) ENVIRONMENT_IS_PTHREAD = false; // ENVIRONMENT_IS_P
 var PthreadWorkerInit; // Collects together variables that are needed at initialization time for the web workers that host pthreads.
 if (!ENVIRONMENT_IS_PTHREAD) PthreadWorkerInit = {};
 var currentScriptUrl = (typeof document !== 'undefined' && document.currentScript) ? document.currentScript.src : undefined;
+#else
+var ENVIRONMENT_IS_PTHREAD = false;


I agree in principle but here's where we have a tradeoff. Gating the 2 lines of ENVIRONMENT_IS_PTHREAD check above on USING_PTHREADS makes the code up there super ugly because of the way the ifdefs interact with conditionals in the code. I went ahead and did it, but I guess apropos of our discussion in #5732 it would be much much more readable to either (for example) turn ENVIRONMENT_IS_PTHREAD into a macro, or go all-in on closure and make everything regular JS code.

kripken · 2017-11-17T16:32:03Z

I had an idea, instead of adding , also_wasm=True to all those tests, could we apply that behavior if an env var is set? Then to run these tests, we'd do something like EMCC_BROWSER_ALSO_WASM=1 ./tests/runner.py browser.test_pthread*

dschuff · 2017-11-21T19:02:46Z

I think having a way to make that the default behavior makes sense, although I also think having the flag makes sense as a way to disable it. But making it on instead of off by default does seem better.

kripken · 2017-11-21T19:15:17Z

I think it makes sense to have it on by default eventually, but maybe only when canary/nightly/etc. can run wasm-threads tests by default, which I don't think is the case yet? So existing waterfalls would need updating to disable the tests.

dschuff · 2017-11-21T21:27:28Z

Sorry, by "on by default" I meant the btest function's also_wasm argument defaults to True (thus allowing callers to set it to False), but also gate running any of the wasm thread tests behind an env var.

kripken · 2017-11-21T22:20:52Z

In that case, it sounds like setting also_wasm to False is similar to the @no_wasm decorator?

dschuff · 2017-11-29T00:51:06Z

It's similar. With the core tests we run the whole suite in a mode (asm2g, binaryen0, etc) which could be wasm or not, (and the flag is handled separately from the tests) and @no_wasm` skips the entire test case if it is. Currently the browser test system doesn't have the same notion of modes, so the test cases just exercise all the flags they want. Part of the reason is that the core tests want to exercise different compiler modes across a whole bunch of tests, whereas the browser tests are mostly trying to exercise the runtime, which is less affected by the compiler modes Therefore we don't want to run the whole suite across all modes because it would be redundant and slow. We could try to make the browser test model more like core; whether that makes sense probably depends on how much the things that the suite wants to exercise vary across the modes. My intuition is that it wouldn't be worth it. btw threads are a notable exception because they can't currently be tested in the shell. Really we would want to put most of those tests in core (if there were a good way to use workers in shells/node).

kripken · 2017-11-29T00:54:28Z

Fair enough, we can leave test improvements for some other time.

lars-t-hansen · 2017-11-29T08:10:28Z

btw threads are a notable exception because they can't currently be tested in the shell. Really we would want to put most of those tests in core (if there were a good way to use workers in shells/node).

Working on it: WebAssembly/threads#52

dschuff · 2017-11-30T22:43:18Z

OK I think i've addressed all the comments and have checked that nothing existing breaks. Shoudl be good, but I'll wait for CI. The circleci build seems to have completed without doing anything; is there something I need to do to make that work?

kripken · 2017-11-30T23:06:19Z

CircleCI will work once you get the changes on incoming, but not worth merging incoming into here for that, as they overlap the travis tests now. (Although they are faster, so it might actually be worth it ;)

Please run the browser tests locally, though, as they don't run on the bots.

dschuff · 2017-12-01T17:49:20Z

OK, verified no new breakage.

kripken · 2017-12-01T21:17:20Z

Great!

Do you need to keep the branch around, or can it be deleted?

dschuff · 2017-12-05T00:43:42Z

Deleted.

dschuff added 7 commits October 24, 2017 16:02

hacks to enable wasm threads

146a970

hack around lack of postMessage for memory

d46fbcb

hack for posting SAB instead of memory

d54cbc9

cleanups and TODOs for hack version

390fe20

remove buffer hacks

068ace5

turn on a bunch of tests

df41327

remove debug print

3c1d59c

dschuff commented Oct 25, 2017

View reviewed changes

kripken requested a review from juj October 25, 2017 18:36

kripken reviewed Oct 25, 2017

View reviewed changes

dschuff added 3 commits October 26, 2017 09:04

Merge branch 'incoming' into threads-hack

6632faa

Merge branch 'incoming' into threads-hack

16bd5ed

use Module.asm instead of Module.wasmModule

8049139

dschuff added 8 commits November 6, 2017 09:50

use --enable-threads in llc flags

8eb4939

Revert "use Module.asm instead of Module.wasmModule"

a17f956

This reverts commit 8049139.

remove async assert hack. pass module to receiveInstance

3791916

dont add run dep in a pthread

9d97628

Factor test changes into also_wasm arg to btest

78ca9fe

Switch to env var to enable pthread wasm tests

a343116

add ENVIRONMENT_IS_PTHREAD to shell.js

6eb49ab

fix ENVIRONMENT_IS_PTHREAD for realz

040d67f

bvibber mentioned this pull request Nov 16, 2017

Merge incoming into modular-pthread bvibber/emscripten#1

Closed

juj reviewed Nov 16, 2017

View reviewed changes

dschuff added 2 commits November 16, 2017 14:15

Merge branch 'incoming' into threads-hack

647dae5

gate more things on WASM and USE_PTHREADS

c2d0251

dschuff commented Nov 17, 2017

View reviewed changes

Merge branch 'incoming' into threads-hack

58ebe68

also_wasm on by default

204cf0a

dschuff added 4 commits November 30, 2017 11:13

we dont have preprocessor on pthread-main.js

b34c3de

factor out exit_with_error

c446849

Require WASM_MEM_MAX when allowing memory growth

e2d17ce

assert is not defined before importScripts

e040b65

dschuff added 2 commits November 30, 2017 16:08

fix browser detection

eba82eb

fix for running browser tests without browser

a71350e

dschuff merged commit dee8676 into incoming Dec 1, 2017

dschuff deleted the threads-hack branch December 5, 2017 00:43

chrisber mentioned this pull request Dec 20, 2017

USE_PTHREADS=1 fails with LLVM ERROR: Cannot select ... #5956

Closed

sbc100 mentioned this pull request Jun 28, 2023

Pth not supported #4086

Closed

Initial support for pthreads with wasm #5710

Initial support for pthreads with wasm #5710

Conversation

dschuff commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Oct 25, 2017

kripken commented Nov 1, 2017 • edited Loading

juj commented Nov 2, 2017

juj commented Nov 6, 2017

dschuff commented Nov 16, 2017

kripken commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Nov 17, 2017

dschuff commented Nov 21, 2017

kripken commented Nov 21, 2017

dschuff commented Nov 21, 2017

kripken commented Nov 21, 2017

dschuff commented Nov 29, 2017

kripken commented Nov 29, 2017

lars-t-hansen commented Nov 29, 2017

dschuff commented Nov 30, 2017

kripken commented Nov 30, 2017

dschuff commented Dec 1, 2017

kripken commented Dec 1, 2017

dschuff commented Dec 5, 2017

dschuff Nov 15, 2017 •

edited

Loading

kripken commented Nov 1, 2017 •

edited

Loading