-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Replace Module["asm"]
with wasmExports
global
#19816
Conversation
If we think its necessary we could consider continuing to support the old |
14f3f38
to
5a3b9e8
Compare
I added support for a |
@@ -382,6 +382,7 @@ function exportRuntime() { | |||
'abort', | |||
'keepRuntimeAlive', | |||
'wasmMemory', | |||
'wasmExports', |
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 this needed? The explicit maybeExport in the proper places handles it, no?
src/parseTools.js
Outdated
@@ -865,6 +865,13 @@ function hasExportedSymbol(sym) { | |||
return WASM_EXPORTS.has(sym); | |||
} | |||
|
|||
function maybeExport(sym) { |
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 is this related to the function here with the same name?
Line 347 in 3e780ad
function maybeExport(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.
Good point. The one in module.js is local to the exportRuntime
so is not public/accessible. Its also has some logic for renaming/legacy support that we don't need in this case. I'm not sure its wroth trying to combine them
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.
Perhaps a different name would be helpful, at least, to avoid confusion? Can maybe rename the other one if that's simpler, to maybe maybeExportAndCheck
("check" for that other renaming/legacy stuff you mentioned..?)
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 renamed the new function receivedSymbol
for now, and added a comment.
We could potentially clean this up as a followup by introducing the concept of a delayed export (i.e. and export that cannot be set until after instantiation. But for now I think this change is big enough.
a1d149f
to
00e9f24
Compare
This brings the regular runtime closer `MINIMAL_RUNTIME` which uses the global `asm` to refer to exports object. As a followup we should consider merging these two symbols.
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 don't have a better idea for a name for the new function... if we think of one maybe we can refactor later. Looks great for now.
I missed a few cases back in #19816. This change completely the transition saving even more on code size.
I missed a few cases back in #19816. This change completely the transition saving even more on code size.
This matches the name used by the regualar runtime as of #19816 and removes one of them major differences between the two runtimes.
I missed a few cases back in #19816. This change completely the transition saving even more on code size.
This matches the name used by the regualar runtime as of #19816 and removes one of them major differences between the two runtimes.
I missed a few cases back in #19816. This change completely the transition saving even more on code size.
This matches the name used by the regualar runtime as of #19816 and removes one of them major differences between the two runtimes.
This matches the name used by the regualar runtime as of #19816 and removes one of them major differences between the two runtimes.
I missed a few cases back in #19816. This change completely the transition saving even more on code size.
This matches the name used by the regualar runtime as of #19816 and removes one of them major differences between the two runtimes.
This matches the name used by the regualar runtime as of #19816 and removes one of them major differences between the two runtimes.
Module.asm was removed, use wasmExports instead. Context: emscripten-core/emscripten#19816
* [wasm] Bump emscripten to 3.1.56 * Replace Module.asm with Module.wasmExports Module.asm was removed, use wasmExports instead. Context: emscripten-core/emscripten#19816 * Updates for .native.worker.js -> mjs rename Context: emscripten-core/emscripten#21041 * Update deps * Add general testing feed * Update mode deps * Update path * Use current python packages for now, we don't have newer ones The current names 3.1.34 are new enough * Keep using llvm 16 for runtime and aot compiler * Add -Wno-pre-c11-compat only for browser * Temporarily disable version checks to get further * Temporarily disable version checks to get further #2 * Disable -Wunused-command-line-argument * Update emsdk deps * Update icu dependency * Revert "Temporarily disable version checks to get further #2" This reverts commit 3f8834f. * Revert "Temporarily disable version checks to get further" This reverts commit fe1e5c6. * Fix emsdk check We use system python on osx too * Workaround wasm-opt crash * Workaround wasm-opt crash * Workaround wasm-opt crash * Fix WBT test * Feedback * Update ICU dependency * Update emscripten deps * Revert "Workaround wasm-opt crash" This reverts commit 200cf3b. * Revert "Workaround wasm-opt crash" This reverts commit 4530edf. * Revert "Workaround wasm-opt crash" This reverts commit 3593c41. * Increase tests timeout * Show test progress * Increase MT library tests timeout * Disable WBT tests with SkiaSharp * Increase helix tests timeout on browser * Increase WBT timeout * Increase initial heap sizes * Fix mono_wasm_load_runtime cwrap signature Fixes: `Uncaught ExitStatus: Assertion failed: stringToUTF8Array expects a string (got number)` * Enable XunitShowProgress for threading tasks tests * Try to reduce number of parallel AOT compilations To check whether it will improve memory issues on CI * Use new docker image for helix/windows tests * Revert "Try to reduce number of parallel AOT compilations" This reverts commit 5d9a6d2. * Reduce the timeouts * Reduce intitial heap size * use active issues for MT * Remove testing channel from nuget config, update deps * Update emsdk and icu dependencies --------- Co-authored-by: Larry Ewing <[email protected]> Co-authored-by: pavelsavara <[email protected]>
This brings the regular runtime closer
MINIMAL_RUNTIME
which uses the globalasm
to refer to exports object. As a followup we should consider merging these two symbols.This also matches the names of the other globals such as
wasmImports
andwasmMemory
.