Fix wasm loading in workers, and refactor async promise code #5011
Conversation
|
I realized as a nice benefit of this refactoring that we can get rid of the ugly preloading of the wasm binary into Module.wasmBinary in emcc. (And that made me notice some bugs in this PR, since it was just using the preloaded binary... fixed). |
| WebAssembly.instantiate(getBinary(), info).then(function(output) { | ||
| getBinaryPromise().then(function(binary) { | ||
| return WebAssembly.instantiate(binary, info) | ||
| }).then(function(output) { |
There was a problem hiding this comment.
Hey, since working on this, can you change the #if #endif in here to a #if #else #endif to avoid the "unreachable code" console warnings below?
| Module['printErr']('asynchronously preparing wasm'); | ||
| addRunDependency('wasm-instantiate'); // we can't run yet | ||
| WebAssembly.instantiate(getBinary(), info).then(function(output) { | ||
| getBinaryPromise().then(function(binary) { |
There was a problem hiding this comment.
The main .html file is supposed to download the .wasm file. At this point, is it not possible that the .wasm file download is still in progress, but has not finished yet, and therefore getBinaryPromise() will kick off a second parallel download redundantly?
There was a problem hiding this comment.
Not as it currently works - the html downloads the wasm binary before starting to do anything else. Another reason it's good to get rid of that thing ;)
| return ret; | ||
| } | ||
|
|
||
| function getBinary() { |
There was a problem hiding this comment.
Do we still need this getBinary() function as well, or could we just have one getBinaryPromise() function that also contains the content of this function?
There was a problem hiding this comment.
We use it when we need synchronous access to it - for the interpreter, or when async compilation is off. Also, I think it's nice it's separate, conceptually.
|
ping @juj, see responses to comments above, i think this is good to go unless you have further issues. |
|
re-ping @juj |
|
Looks alright to me - the use of the |
|
Good to know about fetch, when they ship wasm on iOS we'll need to track that. |
Fixes #5005. Specifically, this