Repository navigation
[dylink] Refactor loadWebAssemblyModule to avoid retaining wasm bytes. NFC - #28021
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors libdylink.js by extracting $addEmAsm and $addEmJs helpers, and splitting $loadWebAssemblyModule to delegate module instantiation to a new helper function $instantiateModule. It also updates the expected code sizes in the test suite. Feedback on the changes highlights that the refactoring omitted the explicit nulling of the binary buffer (binary = null), which was previously used to prevent memory leaks caused by closures retaining the buffer. It is recommended to restore this cleanup in both the synchronous and asynchronous paths.
|
Thanks Sam — I tested ee7e64e on Linux aarch64 with Node 24.19.0. This preserves the memory fix from #28002. The existing other.test_dlopen_binary_not_retained test passes. As a negative control, removing binary = null from the parent commit makes it fail with 32 MiB retained again. I also checked a separate 32 MiB side module with a lazy import, EM_JS and EM_ASM. Both the parent with #28002 and this refactor retained only 48 bytes of additional ArrayBuffer memory after GC; the broken control retained about 32 MiB. Sync and async loading both passed, using either bytes or a precompiled WebAssembly.Module. The lazy import and both JS helpers still worked after GC. The refactor passed these checks at -O0 and -O2. I agree that binary = null is unnecessary with this structure: instantiateModule's nested callbacks no longer reference binary, the import stubs no longer share loadWebAssemblyModule's outer context, and the eval helpers are outside that scope too. The broader dynamic-linking run finished with 362 passed, 23 skipped and no failures, including pthread and selected wasm64 cases. Looks good to me from the retention side. |
Split `instantiateModule`, `addEmAsm`, and `addEmJs` out of `loadWebAssemblyModule` and replace the async IIFE with `.then()` so that long-lived import stub closures do not capture and retain the side module's `binary` buffer in their shared closure context. This also ends up saving a few bytes of code size which is nice. See: emscripten-core#28002
ee7e64e to
2ef6af2
Compare
|
Best viewed with "ignore-whitespace" to see that this is just a pure refactor. |
loadWebAssemblyModule to avoid retaining wasm bytesloadWebAssemblyModule to avoid retaining wasm bytes. NFC
Split
instantiateModule,addEmAsm, andaddEmJsout ofloadWebAssemblyModuleand replace the async IIFE with.then()so that long-lived import stub closures do not capture and retain the side module'sbinarybuffer in their shared closure context.This also ends up saving a few bytes of code size which is nice.
See: #28002