-
Notifications
You must be signed in to change notification settings - Fork 818
[wasm-split] Make placeholder namespace contain module name #7975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to remove this eventually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 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. InsecondaryName
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 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
secondaryName
part to figure out which secondary module file to load in case a placeholder function is called. For example, if the import module isplaceholder.2
and the import base is3
and the primary wasm file istest.wasm
,when
placeholder_3
function is loaded, the runtime will parseplaceholder.2
and correctly figure out that it should loadtest.2.wasm
.Companion PR: emscripten-core/emscripten#25571, which has to land first.