[wasm-split] Make placeholder namespace contain module name#7975
Conversation
For now we use a single namespace for all placeholder imports. This namespace can be set by `--placeholder-namespace` and if not set it is by default `placeholder`. This changes it so that the namespace is in the format of `placeholder.[secondaryName]`. That `placeholder`, or a user-specified string via `--placeholder-namespace`, is a prefix of the namespace and not the namespace itself. In `secondaryName` part we put the name of secondary modules. To be consistent with that change, I'd like to rename `--placeholder-namespace` to `--placeholder-namespace-prefix`. But in order not to break people who are using the old option, this just adds `--placeholder-namespace-prefix` and keeps `--placeholder-namespace` but make its behavior the same as `--placeholder-namespace-prefix`. This is done to make multi-split module loading work in emscripten. Currently, if the primary module name is `test.wasm` and when a placeholder function is called, the emscripten JS runtime loads `test.deferred.wasm`, which is hardcoded. But as we want emscripten to support multi-split, the runtime has to know which secondary module to load. The companion PR in emscripten (#?????) makes emscripten runtime use that `secondaryName` part to figure out which secondary module file to load in case a placeholder function is called. For example, if the import module is `placeholder.2` and the import base is `3` and the primary wasm file is `test.wasm`, ```wast (import "placeholder.2" "3" (func $placeholder_3 (result i32))) ``` when `placeholder_3` function is loaded, the runtime will parse `placeholder.2` and correctly figure out that it should load `test.2.wasm`. Companion PR:
| ModuleSplitting::Config config; | ||
| setCommonSplitConfigs(config, options); | ||
| config.secondaryFuncs.push_back(std::move(splitFuncs)); | ||
| config.secondaryNames.push_back("deferred"); |
There was a problem hiding this comment.
Is the plan to remove this eventually?
There was a problem hiding this comment.
No, this is to keep the current emscripten behavior, that the secondary module is split with the name [moduleName].deferred.wasm. So in emscripten-core/emscripten#25571, it has
let secondaryFile;
if (moduleName == 'placeholder') { // old format
secondaryFile = wasmBinaryFile.slice(0, -5) + '.deferred.wasm';
} else { // new format
let moduleID = moduleName.split('.')[1];
secondaryFile = wasmBinaryFile.slice(0, -5) + '.' + moduleID + '.wasm';
}The plan is to remove this old format part:
if (moduleName == 'placeholder') { // old format
secondaryFile = wasmBinaryFile.slice(0, -5) + '.deferred.wasm';
}And only do with this part:
let moduleID = moduleName.split('.')[1];
secondaryFile = wasmBinaryFile.slice(0, -5) + '.' + moduleID + '.wasm';And with this, the placeholder has to be named placeholder.deferred to keep this behavior.
There was a problem hiding this comment.
I would be happy to make breaking changes to the existing split workflow (e.g. the name of the secondary module) to minimize differences with the multi-split workflow, but this sounds good for now.
There was a problem hiding this comment.
It is still hardcoding and users should make sure their -o2 option in wasm-split to be [moduleName].deferred.wasm, but this is the case now anyway, so it doesn't change from the user's perspective.
Currently, if the primary module name is `test.wasm` and when a placeholder function is called, the emscripten JS runtime loads `test.deferred.wasm`, which is hardcoded. But as we want emscripten to support multi-split, the runtime has to know which secondary module to load. Binaryen's WebAssembly/binaryen#7975 changes the placeholder import module naming scheme. Currently placeholder's import modules are all `placeholder`: https://github.com/WebAssembly/binaryen/blob/dcc704ce20e4723ae42012039bdb3b84ec2035f9/test/lit/wasm-split/multi-split.wast#L291-L295 But the Binaryen's companion PR will change this to the format of `placeholder.[secondaryName]`. Then Emscripten runtime will parse this module name to learn which secondary file to load. For example, if the import module is `placeholder.2` and the import base is `3` and the primary wasm file is `test.wasm`, ```wast (import "placeholder.2" "3" (func $placeholder_3 (result i32))) ``` when `placeholder_3` function is loaded, the runtime will parse `placeholder.2` and correctly figure out that it should load `test.2.wasm`. This PR still supports the old `placeholder` namespace, because this PR has to land first for the Binaryen companion PR to land. After it lands, we can remove the `placeholder` handling part. Companion PR: WebAssembly/binaryen#7975
|
@aheejin I think the Split fuzzer needs to be updated for this. To quickly see the error, disable all fuzzers but Split in |
After WebAssembly#7975, the default namespace for the two-way split changed to `placeholder.deferred`.
|
Sorry, done in #7989. |
After #7975, the default namespace for the two-way split changed to `placeholder.deferred`.
Currently, if the primary module name is `test.wasm` and when a placeholder function is called, the emscripten JS runtime loads `test.deferred.wasm`, which is hardcoded. But as we want emscripten to support multi-split, the runtime has to know which secondary module to load. Binaryen's WebAssembly/binaryen#7975 changes the placeholder import module naming scheme. Currently placeholder's import modules are all `placeholder`: https://github.com/WebAssembly/binaryen/blob/dcc704ce20e4723ae42012039bdb3b84ec2035f9/test/lit/wasm-split/multi-split.wast#L291-L295 But the Binaryen's companion PR will change this to the format of `placeholder.[secondaryName]`. Then Emscripten runtime will parse this module name to learn which secondary file to load. For example, if the import module is `placeholder.2` and the import base is `3` and the primary wasm file is `test.wasm`, ```wast (import "placeholder.2" "3" (func $placeholder_3 (result i32))) ``` when `placeholder_3` function is loaded, the runtime will parse `placeholder.2` and correctly figure out that it should load `test.2.wasm`. This PR still supports the old `placeholder` namespace, because this PR has to land first for the Binaryen companion PR to land. After it lands, we can remove the `placeholder` handling part. Companion PR: WebAssembly/binaryen#7975
For now we use a single namespace for all placeholder imports. This namespace can be set by
--placeholder-namespaceand if not set it is by defaultplaceholder.This changes it so that the namespace is in the format of
placeholder.[secondaryName]. Thatplaceholder, or a user-specified string via--placeholder-namespace, is a prefix of the namespace and not the namespace itself. InsecondaryNamepart we put the name of secondary modules.To be consistent with that change, I'd like to rename
--placeholder-namespaceto--placeholder-namespace-prefix. But in order not to break people who are using the old option, this just adds--placeholder-namespace-prefixand keeps--placeholder-namespacebut make its behavior the same as--placeholder-namespace-prefix.This is done to make multi-split module loading work in emscripten. Currently, if the primary module name is
test.wasmand when a placeholder function is called, the emscripten JS runtime loadstest.deferred.wasm, which is hardcoded. But as we want emscripten to support multi-split, the runtime has to know which secondary module to load.The companion PR in emscripten (emscripten-core/emscripten#25571) makes emscripten runtime use that
secondaryNamepart to figure out which secondary module file to load in case a placeholder function is called. For example, if the import module isplaceholder.2and the import base is3and the primary wasm file istest.wasm,when
placeholder_3function is loaded, the runtime will parseplaceholder.2and correctly figure out that it should loadtest.2.wasm.Companion PR: emscripten-core/emscripten#25571, which has to land first.