WIP: Add preload plugin to compile wasm side modules async#6663
WIP: Add preload plugin to compile wasm side modules async#6663kripken merged 13 commits intoemscripten-core:incomingfrom
Conversation
| Module['preloadPlugins'].push(audioPlugin); | ||
|
|
||
| #if WASM | ||
| var wasmPlugin = {}; |
There was a problem hiding this comment.
please also put this behind the MAIN_MODULE flag, so it's not present in code that doesn't support shared modules.
src/library_browser.js
Outdated
|
|
||
| #if WASM | ||
| var wasmPlugin = {}; | ||
| wasmPlugin['asyncWasmLoadPromise'] = new Promise((resolve, reject) => resolve()); |
There was a problem hiding this comment.
I think => is ES6 notation that currently breaks our JS optimizer. Until we fix that, have to use ES5, sorry...
src/library_browser.js
Outdated
| var wasmPlugin = {}; | ||
| wasmPlugin['asyncWasmLoadPromise'] = new Promise((resolve, reject) => resolve()); | ||
| wasmPlugin['canHandle'] = function(name) { | ||
| return !Module.noWasmDecoding && (name.endsWith('.so') || name.endsWith('.wasm')); |
There was a problem hiding this comment.
I copied the pattern from the audio and image decoders above. It's a way for users to turn off the decoder if desired, I suppose. It might make sense to opt-in rather than opt-out of this, but that would be a change from the other two preload plugins. Happy to change if it makes sense to.
|
|
||
| Module["preloadedImages"] = {}; // maps url to image data | ||
| Module["preloadedAudios"] = {}; // maps url to audio data | ||
| Module["preloadedWasm"] = {}; // maps url to wasm instance exports |
There was a problem hiding this comment.
please put this behind the WASM and MAIN_MODULE flags
(also, we should probably put the others behind a flag for preload-plugins, not sure if we have one yet)
src/support.js
Outdated
| #if WASM | ||
| // Loads a side module from binary data | ||
| function loadWebAssemblyModule(binary) { | ||
| function loadWebAssemblyModule(binary, load_async) { |
93a1f6b to
5a42391
Compare
src/library_browser.js
Outdated
| }; | ||
| Module['preloadPlugins'].push(audioPlugin); | ||
|
|
||
| #if WASM && MAIN_MODULE |
There was a problem hiding this comment.
our preprocessor is pretty simple, and doesn't support &&... can nest one if inside another.
There was a problem hiding this comment.
(I think that explains the CI errors)
src/library_browser.js
Outdated
| }; | ||
| Module['preloadPlugins'].push(audioPlugin); | ||
|
|
||
| #if (WASM != 0) && (MAIN_MODULE != 0) |
There was a problem hiding this comment.
I don't believe this works. Do you see it prevent the code from being included when it should not be?
All I am sure works is
#if WASM
#if MAIN_MODULE
...
#endif
#endif
Sorry about the ugliness, we should improve the preprocessor (which is literally just a few lines of JS at this point)
kripken
left a comment
There was a problem hiding this comment.
Mostly just minor comments, overall looks nice.
We also need a test for this.
src/library_browser.js
Outdated
| // promises to run in series. | ||
| this.asyncWasmLoadPromise = this.asyncWasmLoadPromise.then( | ||
| function() { | ||
| return Module.loadWebAssemblyModule(byteArray, true) |
There was a problem hiding this comment.
loadWebAssemblyModule should be quoted.
However, does it need to be on Module at all? I think you can just call loadWebAssemblyModule() here.
src/library_browser.js
Outdated
| // loadWebAssemblyModule can not load modules out-of-order, so rather | ||
| // than just running the promises in parallel, this makes a chain of | ||
| // promises to run in series. | ||
| this.asyncWasmLoadPromise = this.asyncWasmLoadPromise.then( |
There was a problem hiding this comment.
For closure, if a property is quoted once, it must be quoted in all uses, so the two uses on this line must be quoted
src/library_browser.js
Outdated
| // promises to run in series. | ||
| this.asyncWasmLoadPromise = this.asyncWasmLoadPromise.then( | ||
| function() { | ||
| return Module.loadWebAssemblyModule(byteArray, true) |
|
Quick question about writing a test. I added a test to This is coming from the code that I had assumed |
778034a to
fe094dc
Compare
|
How did you write the test? If you call emcc etc. yourself, it doesn't know it's a browser test (even if it's in |
|
Thanks for the tip. Making progress. I have a test mostly working (pushed to this PR). It displays the expected output in the browser window, but then hangs for a long time and it seems the test harness doesn't get the output. |
tests/test_browser.py
Outdated
| #endif | ||
| } | ||
| ''') | ||
| check_execute([PYTHON, EMCC, 'library.c', '-s', 'SIDE_MODULE=1', '-O2', '-o', 'library.wasm', '-s', 'WASM=1']) |
There was a problem hiding this comment.
we prefer run_process now for calling python processes
|
Ah, another difference is we don't forward stdout/stderr in browser tests. Instead, use the |
|
Cool. Thanks for the pointers. I think I have something that works now... we'll see how the CI likes it. |
|
Thanks! Looks good on CI (some other random error there, but doesn't look related). However, reading the test now, it makes me think maybe a preload plugin on |
|
Another thing we should do here is update the docs to mention this. I think we have something about preload-plugins somewhere, but I don't remember offhand. |
|
wrt file extensions: wrt documentation: This is the closest thing I can find in the docs, and even it doesn't go into much detail. Should I write a preloading file section from scratch? |
|
Ok, let's do For docs, yeah - it seems like we don't have a good section on this then. I think the best place to add a subsection is in https://kripken.github.io/emscripten-site/docs/porting/files/packaging_files.html , and please link to it from the place you found, as well as the preloading-related functions mentioned in https://kripken.github.io/emscripten-site/docs/api_reference/emscripten.h.html Thanks! |
ce3f9bb to
6b79174
Compare
|
Great. I've made this |
|
Looks good, thanks! Just waiting on CI to re-run (was some problem on the browser run, which is important here). |
|
Actually (There are also 2 emrun-using tests that fail as well, but they are breakage on incoming, should be fixed there now, can ignore those.) |
|
Ah, the test broke because it was testing a SIDE_MODULE with a |
|
Looks good, thanks! |
Currently Emscripten allows to create shared libraries (DSO) and link them to main module. However a shared library itself cannot be linked to another shared library. The lack of support for DSO -> DSO linking becomes problematic in cases when there are several shared libraries that all need to use another should-be shared functionality, while linking that should-be shared functionality to main module is not an option for size reasons. My particular use-case is SciPy support for Pyodide: pyodide/pyodide#211, pyodide/pyodide#240 where several of `*.so` scipy modules need to link to LAPACK. If we link to LAPACK statically from all those `*.so` - it just blows up compiled size pyodide/pyodide#211 (comment) and if we link in LAPACK statically to main module, the main module size is also increased ~2x, which is not an option, since LAPACK functionality is not needed by every Pyodide user. This way we are here to add support for DSO -> DSO linking: 1. similarly to how it is already working for main module -> side module linking, when building a side module it is now possible to specify -s RUNTIME_LINKED_LIBS=[...] with list of shared libraries that side module needs to link to. 2. to store that information, for asm.js, similarly to how it is currently handled for main module (which always has js part), we transform RUNTIME_LINKED_LIBS to libModule.dynamicLibraries = [...] (see src/preamble_sharedlib.js) 3. for wasm module, in order to store the information about to which libraries a module links, we could in theory use "module" attribute in wasm imports. However currently emscripten almost always uses just "env" for that "module" attribute, e.g. (import "env" "abortStackOverflow" (func $fimport$0 (param i32))) (import "env" "_ffunc1" (func $fimport$1)) ... and this way we have to embed the information about required libraries for the dynamic linker somewhere else. What I came up with is to extend "dylink" section with information about which shared libraries a shared library needs. This is similar to DT_NEEDED entries in ELF. (see tools/shared.py) 4. then, the dynamic linker (loadDynamicLibrary) is reworked to handle that information: - for asm.js, after loading a libModule, we check libModule.dynamicLibraries and post-load them recursively. (it would be better to load needed modules before the libModule, but for js we currently have to first eval whole libModule's code to be able to read .dynamicLibraries) - for wasm the needed libraries are loaded before the wasm module in question is instantiated. (see changes to loadWebAssemblyModule for details) 5. since we also have to teach dlopen() to handle needed libraries, and since dlopen was already duplicating loadDynamicLibrary() code in many ways, instead of adding more duplication, dlopen is now reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)"). (see changes to dlopen and loadDynamicLibrary) 6. The functionality to asynchronously load dynamic libraries is also integrated into loadDynamicLibrary. Libraries were asynchronously preloaded for the case when Module['readBinary'] is absent (browser, see 3446d2a "preload wasm dynamic libraries when we can't load them synchronously"). Since this codepath was also needed to be taught of DSO -> DSO dependency, the most straightforward thing to do was to teach loadDynamicLibrary to do its work asynchronously (under flag) and to switch the preloading to use loadDynamicLibrary(..., {loadAsync: true}) (see changes to src/preamble.js and loadDynamicLibrary) 7. A test is added for verifying linking/dlopening a DSO with other needed library. browser.test_dynamic_link is also amended to verify linking to DSO with dependencies. With the patch I've made sure that all core tests (including test_dylink_* and test_dlfcn_*) are passing for asm{0,1,2} and binaryen{0,1,2}. However since I cannot get full browser tests to pass even on pristine incoming (1.38.19-2-g77246e0c1 as of today; there are many failures for both Firefox 63.0.1 and Chromium 70.0.3538.67), I did not tried to verify full browser tests with my patch. Bit I've made sure that browser.test_preload_module browser.test_dynamic_link are passing. "other" kind of tests also do not pass on pristine incoming for me. This way I did not tried to verify "other" with my patch. Thanks beforehand, Kirill P.S. This is my first time I do anything with WebAssembly/Emscripten, and only a second time with JavaScript, so please forgive me if I missed something. P.P.S. I can split the patch into smaller steps, if it will help review. /cc @kripken, @juj, @sbc100, @max99x, @junjihashimoto, @mdboom, @rth
Currently Emscripten allows to create shared libraries (DSO) and link them to main module. However a shared library itself cannot be linked to another shared library. The lack of support for DSO -> DSO linking becomes problematic in cases when there are several shared libraries that all need to use another should-be shared functionality, while linking that should-be shared functionality to main module is not an option for size reasons. My particular use-case is SciPy support for Pyodide: pyodide/pyodide#211, pyodide/pyodide#240 where several of `*.so` scipy modules need to link to LAPACK. If we link to LAPACK statically from all those `*.so` - it just blows up compiled size pyodide/pyodide#211 (comment) and if we link in LAPACK statically to main module, the main module size is also increased ~2x, which is not an option, since LAPACK functionality is not needed by every Pyodide user. This way we are here to add support for DSO -> DSO linking: 1. similarly to how it is already working for main module -> side module linking, when building a side module it is now possible to specify -s RUNTIME_LINKED_LIBS=[...] with list of shared libraries that side module needs to link to. 2. to store that information, for asm.js, similarly to how it is currently handled for main module (which always has js part), we transform RUNTIME_LINKED_LIBS to libModule.dynamicLibraries = [...] (see src/preamble_sharedlib.js) 3. for wasm module, in order to store the information about to which libraries a module links, we could in theory use "module" attribute in wasm imports. However currently emscripten almost always uses just "env" for that "module" attribute, e.g. (import "env" "abortStackOverflow" (func $fimport$0 (param i32))) (import "env" "_ffunc1" (func $fimport$1)) ... and this way we have to embed the information about required libraries for the dynamic linker somewhere else. What I came up with is to extend "dylink" section with information about which shared libraries a shared library needs. This is similar to DT_NEEDED entries in ELF. (see tools/shared.py) 4. then, the dynamic linker (loadDynamicLibrary) is reworked to handle that information: - for asm.js, after loading a libModule, we check libModule.dynamicLibraries and post-load them recursively. (it would be better to load needed modules before the libModule, but for js we currently have to first eval whole libModule's code to be able to read .dynamicLibraries) - for wasm the needed libraries are loaded before the wasm module in question is instantiated. (see changes to loadWebAssemblyModule for details) 5. since we also have to teach dlopen() to handle needed libraries, and since dlopen was already duplicating loadDynamicLibrary() code in many ways, instead of adding more duplication, dlopen is now reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)"). (see changes to dlopen and loadDynamicLibrary) 6. The functionality to asynchronously load dynamic libraries is also integrated into loadDynamicLibrary. Libraries were asynchronously preloaded for the case when Module['readBinary'] is absent (browser, see 3446d2a "preload wasm dynamic libraries when we can't load them synchronously"). Since this codepath was also needed to be taught of DSO -> DSO dependency, the most straightforward thing to do was to teach loadDynamicLibrary to do its work asynchronously (under flag) and to switch the preloading to use loadDynamicLibrary(..., {loadAsync: true}) (see changes to src/preamble.js and loadDynamicLibrary) 7. A test is added for verifying linking/dlopening a DSO with other needed library. browser.test_dynamic_link is also amended to verify linking to DSO with dependencies. With the patch I've made sure that all core tests (including test_dylink_* and test_dlfcn_*) are passing for asm{0,1,2} and binaryen{0,1,2}. However since I cannot get full browser tests to pass even on pristine incoming (1.38.19-2-g77246e0c1 as of today; there are many failures for both Firefox 63.0.1 and Chromium 70.0.3538.67), I did not tried to verify full browser tests with my patch. Bit I've made sure that browser.test_preload_module browser.test_dynamic_link are passing. "other" kind of tests also do not pass on pristine incoming for me. This way I did not tried to verify "other" with my patch. Thanks beforehand, Kirill P.S. This is my first time I do anything with WebAssembly/Emscripten, and only a second time with JavaScript, so please forgive me if I missed something. P.P.S. I can split the patch into smaller steps, if it will help review. /cc @kripken, @juj, @sbc100, @max99x, @junjihashimoto, @mdboom, @rth
Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").
Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").
Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").
Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").
Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").
Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").
Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").
…it (#7571) Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (#6663)").
This is a possible solution to #6633 related to dlopen'ing SIDE_MODULES using async wasm compilation (required by Chromium).
This adds a new preload handler for
.soand.wasmfiles to compile them using the async API (WebAssembly.instantiate) at data-loading time. The actual dynamic linking happens synchronously when callingdlopenlater. The downside here is that you may end up compiling wasm that never actually gets dlopen'ed, but I think that's probably a domain-specific problem and will be up to the user to resolve, ultimately.The only change required on the user end is to add
--use-preload-pluginswhen file packaging.Of note is that
loadWebAssemblyModulemost be called one at a time, since it sets up an environment in global variables that is later used when the wasm is compiled asynchronously. So this uses a chain of promises to do the compilation, rather than justPromise.all.This will need tests, obviously, but I thought I'd get feedback on the general approach first.