-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[SPLIT_MODULE] Make getWasmImports return Proxy #25559
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
When `SPLIT_MODULE` is set, this makes the `imports` object returned by `getWasmImports` itself a Proxy, which redirects property requests starting with `placeholder` to an inner `splitModuleProxyHandler` that processes the second module loading, and process all other property requests to the original `imports` object itself. This is a prepration to add multi-split loading functionality to the JS runtime. In the multi-split mode, we don't have a single seconday module `[modulename].deferred.wasm`, but instead multiple secondary modules. We plan to have multiple placeholder namespace (e.g., `placeholder_2`, `placeholder_3`, ...) that match those multiple files, and this can't be hardcoded with one `placeholder` string as we currently do, so having the `imports` object itself as a `Proxy` provides us flexibility to handle multiple placeholder namespaces and load correct modules later.
"Hide whitespaces" will make the diff easier to view. |
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (7) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_cxx_wasmfs.json: 176780 => 176780 [+0 bytes / +0.00%] test/codesize/test_codesize_file_preload.expected.js updated codesize/test_codesize_files_wasmfs.json: 55683 => 55683 [+0 bytes / +0.00%] codesize/test_codesize_hello_O0.json: 39197 => 39189 [-8 bytes / -0.02%] codesize/test_codesize_hello_dylink_all.json: 843553 => 843559 [+6 bytes / +0.00%] test/codesize/test_codesize_minimal_O0.expected.js updated codesize/test_unoptimized_code_size.json: 180236 => 180290 [+54 bytes / +0.03%] Average change: +0.00% (-0.02% - +0.03%) ```
Not sure why the codesize test is failing... I think my local machine's |
Usually updating the emsdk (for LLVM and Binaryen) and merging in latest main here fixes those code size errors, for me. |
src/preamble.js
Outdated
var splitModuleProxyHandler = { | ||
get(target, prop, receiver) { | ||
return (...args) => { | ||
var wasmImportsProxyHandler = { |
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.
Can we keep the splitModule prefix (or at least include it somewhere in the name) so its clear when this needed/used?
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.
Renamed it to splitImportsProxyHandler
: e918916
src/preamble.js
Outdated
loadSplitModule(deferred, imports, prop); | ||
err('instantiated deferred module, continuing'); | ||
// TODO: Implement multi-split module loading | ||
err(`placeholder function called: ${base}`); |
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.
Pre-existing, but should this be dbg
and wrapped in ASSERTIONS
or somesuch?
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.
Used dbg
in two places and wrapped them in RUNTIME_DEBUG
, given that the places where dbg()
is used in this file are all seem to be wrapped in it: 4f2f3b3
I updated both but still somehow does not match the CI's results.... |
Is there a way to log in to the CI via ssh and run things? I used to an option that's called like "run via SSH" or something but I don't see it anymore. Also the message very confusing. The CI failure message is like The following (5) test expectation files were updated by
running the tests with `--rebaseline`:
codesize/test_codesize_cxx_wasmfs.json: 176780 => 176780 [+0 bytes / +0.00%]
codesize/test_codesize_files_wasmfs.json: 55683 => 55683 [+0 bytes / +0.00%]
codesize/test_codesize_hello_O0.json: 39197 => 39189 [-8 bytes / -0.02%]
codesize/test_codesize_hello_dylink_all.json: 843553 => 843559 [+6 bytes / +0.00%]
codesize/test_unoptimized_code_size.json: 180236 => 180212 [-24 bytes / -0.01%]
Average change: -0.01% (-0.02% - +0.00%) But I already updated those files in this PR. For example, you can see this PR updates |
The "Run via SSH" things only works for circleci. Are you sure you did |
src/preamble.js
Outdated
} | ||
}; | ||
#if SPLIT_MODULE | ||
imports = new Proxy(imports, splitImportsProxyHandler); |
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.
How about splitModuleProxyHandler
for the name?
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.
That's the name of the inner handler... Do you want it to change to something else?
return (...args) => { | ||
#if ASYNCIFY == 2 | ||
throw new Error('Placeholder function "' + prop + '" should not be called when using JSPI.'); | ||
throw new Error('Placeholder function "' + base + '" should not be called when using JSPI.'); |
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.
(As an aside I wonder if this line should be an assert).
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.
Not sure... Can't this be triggered by user errors? If so an Error
might be fine?
This reverts commit b2a22d4.
Sorry, reverted an accidental change that was not meant to be in this PR: 68c3308 |
src/preamble.js
Outdated
var splitImportsProxyHandler = { | ||
get(target, moduleName, receiver) { | ||
if (moduleName.startsWith('placeholder')) { | ||
var splitModuleProxyHandler = { |
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.
Just call this one innerHandler
then maybe?
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.
This inner name is less important since its not in the global namespace. It could even just be inlined (anonymous) bat that could make harder to grok maybe?
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.
Renamed the outer handler to splitModuleProxyHandler
and the inner one to innerHandler
: eaea168
The inner handler name is used here, so now sure how to anonymize it.
return new Proxy({}, innerHandler);
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 mean you could just write return new Proxy({}, { ... inner handler code }
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.
Done: 67e5380
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (5) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_cxx_wasmfs.json: 176780 => 176780 [+0 bytes / +0.00%] codesize/test_codesize_files_wasmfs.json: 55683 => 55683 [+0 bytes / +0.00%] codesize/test_codesize_hello_O0.json: 39189 => 39197 [+8 bytes / +0.02%] codesize/test_codesize_hello_dylink_all.json: 843559 => 843553 [-6 bytes / -0.00%] codesize/test_unoptimized_code_size.json: 180290 => 180314 [+24 bytes / +0.01%] Average change: +0.01% (-0.00% - +0.02%) ```
I removed |
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (5) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_cxx_wasmfs.json: 176780 => 176780 [+0 bytes / +0.00%] codesize/test_codesize_files_wasmfs.json: 55683 => 55683 [+0 bytes / +0.00%] codesize/test_codesize_hello_O0.json: 39197 => 39189 [-8 bytes / -0.02%] codesize/test_codesize_hello_dylink_all.json: 843553 => 843559 [+6 bytes / +0.00%] codesize/test_unoptimized_code_size.json: 180314 => 180290 [-24 bytes / -0.01%] Average change: -0.01% (-0.02% - +0.00%) ```
I just landed #25566 which should help with the codesize issues. |
Thanks, but it still fails with the codesize checks... I think the wasm sizes are different and have no clue why. I also tried |
The first thing to do is make sure you can run If you have changes in this case then either:
Maybe you are getting bitten by (3) |
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (1) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_unoptimized_code_size.json: 180212 => 180290 [+78 bytes / +0.04%] Average change: +0.04% (+0.04% - +0.04%) ```
I thought I did all three, but now I do it again, somehow I again see changes... Let's see if this time it works 🙏 |
Codesize checks succeeded! Thanks! |
When
SPLIT_MODULE
is set, this makes theimports
object returned bygetWasmImports
itself a Proxy, which redirects property requests starting withplaceholder
to an inner handler that processes the second module loading, and process all other property requests to the originalimports
object itself.This is a prepration to add multi-split loading functionality to the JS runtime. In the multi-split mode, we don't have a single seconday module
[modulename].deferred.wasm
, but instead multiple secondary modules. We plan to have multiple placeholder namespace (e.g.,placeholder2
,placeholder3
, ...) that match those multiple files, and this can't be hardcoded with oneplaceholder
string as we currently do, so having theimports
object itself as aProxy
provides us flexibility to handle multiple placeholder namespaces and load correct modules later.Thanks @tlively for the idea!