Don't inclue mem init loading code when it is inside the wasm anyhow

[kripken]

No description provided.

[juj]

This is something that we should only do in singlethreaded Wasm builds, because in multithreaded Wasm builds, we currently need the memory initializer, or creating a new thread will reinitialize the data section. (Even if that behavior is fixed in the spec, it will be useful to have a separate memory initializer, since the memory initializer can be thrown away after applied, but the Wasm Module can not)

[kripken]

This should work in multithreaded wasm builds too, assuming they don't include the mem init in the wasm (which sounds like we have to, from what you say).

I also ran the browser tests yesterday and they all passed, do we check this issue there?

[kripken]

@dschuff: do you have any concerns with this PR? @juj was worried about multithreaded wasm builds.

[dschuff]

Yes, this is a good point; IIUC emscripten doesn't currently support using a mem init file with wasm at all, but for multithreaded wasm we will need to use one unless we fix the spec (see WebAssembly/threads#62). For now we should probably just do something like juj@a9b66de

[dschuff]

That's an interesting point about keeping it separate anyway. It would be unfortunate to always have to use a separate file even if the spec allows skipping initialization. In emscripten we have to keep the WebAssembly.Module object alive to fork new threads; presumably the VM's representation of that includes the memory initializer, but you could imagine it having some different treatment of active and inactive segments (see the current version of the proposal for definitions of those terms) e.g. when deserializing a module.

[kripken]

Got it, thanks. I made it force a separate mem init file if pthreads are in use.

Where is that commit of @juj's from btw? Has it landed yet or not?

[dschuff]

I assume the commit was from some working branch of his; it's not in upstream yet. I found it from a cross-reference in WebAssembly/threads#62.

Can you also add the test from there?

Also: IIUC the behavior with this patch will be to (silently) never output a mem file for single threaded wasm, and always output one for pthreads, ignoring the value of --mem-init-file. This requires fewer arguments but the behavior is not obvious; in particular the need to include the mem file to run the program. The other option would be to force the user to specify --mem-init-file=1 for pthreads (and respect the setting for single thread).

From the discussion in WebAssembly/threads#62 the engine implementers are taking into account their ability to optimize the init code (e.g. throw away after use). Given that, it would ultimately be my preference to default to keeping the memory init in a single file, in the long-term wasm-only future. If we do want to support a mem-init-file option (e.g. if the initializers are really big) then it should maybe just be in the form of another wasm module imported by the main module. On the first instantiation It would initialize the data section via whatever mechanism we end up standardizing, without the need for any emscripten/JS code.

[dschuff]

Oh, so i didn't address the question of "what do we do for right now, until the JS engines implement whatever we decide on". I guess It kind of doesn't matter since we'll change it; since I prefer keeping the data section in the wasm module by default, I guess I'd say just go with this PR for now, since then a hypothetical user who uses the same flags (i.e. no --mem-init-file) would get the new desired behavior automatically.

So, LGTM if you add the test.

[kripken]

Makes sense, yeah, sounds like we'll need to change this eventually, so for now I'll add that test and merge this.