Repository navigation
Avoid copying all exports in applySignatureConversions - #27996
rustyconover wants to merge 3 commits into
Conversation
When exports need wrapping for pointer conversions (`MEMORY64`, or `MAXIMUM_MEMORY` over 2GB) `applySignatureConversions` used to start by copying the entire exports object, and then replace the handful of exports that actually need wrappers. For programs with many exports (e.g. `MAIN_MODULE=1`, which exports everything) the copy dominates: with ~48k exports it takes ~11ms on an M3 (~39ms on an EC2 instance) in order to wrap ~350 functions. Instead, we now put just the wrapped exports on a new object, and set its prototype to the original exports object so that all other exports are inherited. The prototype has to be set after the wrappers are assigned: the original exports object is frozen, and assignment can't shadow an inherited read-only property. The original exports are read via a new `origExports` local, which the minify-import-export-names pass now also renames. The only consumer that enumerated the returned object was the `DECLARE_ASM_MODULE_EXPORTS=0` version of `assignWasmExports`, which used `Object.entries` (own properties only). It now uses `for...in`. When there is nothing to wrap the function now just returns its input. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
93162ab to
d458880
Compare
| wasmExports.add_one: 42 | ||
| enumerated: 3 | ||
| ''' | ||
| self.do_runf('main.c', expected, cflags=['-sALLOW_MEMORY_GROWTH', '-sMAXIMUM_MEMORY=4GB', '-sSIGNATURE_CONVERSIONS=get_high_ptr:p'] + args) |
There was a problem hiding this comment.
Don't we already have tests for this elsewhere?
There was a problem hiding this comment.
The wrapping itself is covered (core_2gb, wasm64), but I couldn't find coverage for the two paths this PR changes, and the test suites agree. With the acorn-optimizer.mjs change reverted, all 2374 tests in wasm64_O2 + core_2gb give the same results as with it; only the O3 variant here fails. Nothing else combines wrapping with DECLARE_ASM_MODULE_EXPORTS=0, so the no_declare_asm_module_exports variant is the only thing that catches the libcore.js change. If you'd rather not have a new test, I could instead add 4GB variants to test_no_declare_asm_module_exports and to an optimized test that minifies export names. Let me know which you prefer.
There was a problem hiding this comment.
I'm not to concerned about adding a new test here if it makes sense.
We do have several tests that use -sDECLARE_ASM_MODULE_EXPORTS=0 in test_core.py.. maybe tho issue is that we don't have a wasm64_O3 configuration? Should we add one maybe? (Possibly in addition to this new test).
There was a problem hiding this comment.
You're right, and I was wrong earlier: test_no_declare_asm_module_exports does catch the libcore.js change (it fails under wasm64, wasm64_4gb and core_2gb without it). I've removed that variant from the new test in ffe1224.
A wasm64_O3 config would catch the acorn change. Export names are only minified at -O3, so wasm64_O2 can't see it. I tried it locally: 841 of its 1187 tests fail with the acorn change reverted. With this PR, 12 fail, and the same 12 fail under wasm64_O2 on main for me.
I can add wasm64_O3 in a follow-up PR, if that sounds good.
- `isExportUse` matches both `wasmExports` and `origExports`, so callers only check once. - Always end `applySignatureConversions` with `return wasmExports`, and set the prototype as a separate statement when wrapping. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
test_no_declare_asm_module_exports already covers the libcore.js change under wasm64, wasm64_4gb and core_2gb. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@sbc100 CI failure seems unrelated? |
|
@sbc100 CI is green now. Could this be merged when you have a moment? |
|
Can you propose a good commit message for this change? Either here, or my update the PR description. |
rustyconover
changed the title
| # then inherits all the others from the original. The wrappers can't be | ||
| # assigned to an object that already inherits from the original exports | ||
| # object, since that is frozen, so we set the prototype afterwards. | ||
| wrappers += '\n var origExports = wasmExports;\n wasmExports = {};' |
There was a problem hiding this comment.
How about wasmExports = Object.create(origExports); (then we don't need the setPrototypeOf line?)
|
Hi @sbc100, The catch is that We could use If you disagree or I'm wrong let me know, Rusty |
applySignatureConversions copied the entire exports object just to wrap
the few functions that need pointer conversions. Instead, build an object
holding only the wrappers and give it the original exports as its
prototype. With 40k exports this step goes from ~39 ms to ~2 ms.
This only applies when wrapping happens (MEMORY64, or MAXIMUM_MEMORY over
2GB). In that case Object.keys(Module['wasmExports']) now lists only the
wrapped exports; lookups by name,
in, and for...in are unchanged. Theexport-name minifier now also renames
origExports.