Skip to content
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

Move sharedModules object to runtime_pthread.js #23998

Merged
merged 8 commits into from
Mar 27, 2025

Conversation

kleisauke
Copy link
Collaborator

No description provided.

@kleisauke
Copy link
Collaborator Author

For some reason this fixes a regression after commit a14f471. After this commit, my pet project started failing with:

TypeError: Cannot read properties of undefined (reading 'vips-jxl.wasm')
    at e (file:///home/kleisauke/wasm-vips/lib/vips-node.mjs:44:61)
    at f (file:///home/kleisauke/wasm-vips/lib/vips-node.mjs:44:402)
    at zf (file:///home/kleisauke/wasm-vips/lib/vips-node.mjs:45:263)
    at file:///home/kleisauke/wasm-vips/lib/vips-node.mjs:46:305

At this line:

var sharedMod = sharedModules[libName];

This could probably(?) also be resolved by moving this object to preamble.js, but this approach felt neater.

(added this as a second comment since the PR description is nowadays used in the landing commit)

@sbc100
Copy link
Collaborator

sbc100 commented Mar 27, 2025

So it makes sense that #23704 could have caused this kind of issue, because it makes means that postable code run only after the createWasm is resolved.

However, I'd love to know why none of our tests caught this. Maybe we don't have enough coverage of dynamic linking + modularize + pthreads?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 27, 2025

I think maybe a better place for this would be in src/runtime_pthread.js at the top (around line 15).

@kleisauke kleisauke changed the title Move sharedModules object to libcore.js Move sharedModules object to runtime_pthread.js Mar 27, 2025
@kleisauke
Copy link
Collaborator Author

Maybe we don't have enough coverage of dynamic linking + modularize + pthreads?

I think so. It might also be a Closure-specific issue since I'm using that as well.

I think maybe a better place for this would be in src/runtime_pthread.js at the top (around line 15).

Done with ba18f68. I confirm that works too.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although I'd love to also get a test for this.

Maybe test_preload_module could have also_with_modularize added?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 27, 2025

LGTM although I'd love to also get a test for this.

Maybe test_preload_module could have also_with_modularize added?

Actually maybe not that test.. but any test that just used load-time dynamic linking and pthreads.

@kleisauke
Copy link
Collaborator Author

Added a test with commit 3854ac7. The _modularize variant of this test would previously fail with:

/tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2546
        var sharedMod = sharedModules[libName];
                                     ^

TypeError: Cannot read properties of undefined (reading 'side.wasm')
    at loadLibData (/tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2546:38)
    at getExports (/tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2583:18)
    at loadDynamicLibrary (/tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2600:16)
    at /tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2641:11
Thrown at:
    at loadLibData (/tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2546:38)
    at getExports (/tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2583:18)
    at loadDynamicLibrary (/tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2600:16)
    at /tmp/emtest_pg577vro/emscripten_test_other_kb_3mt0y/main.js:2641:11

This is an automatic change generated by tools/maint/rebaseline_tests.py.

The following (25) test expectation files were updated by
running the tests with `--rebaseline`:

```
code_size/embind_hello_wasm.json: 16994 => 16994 [+0 bytes / +0.00%]
code_size/embind_val_wasm.json: 16647 => 16647 [+0 bytes / +0.00%]
code_size/hello_webgl2_wasm.json: 13122 => 13122 [+0 bytes / +0.00%]
code_size/hello_webgl2_wasm2js.json: 18420 => 18420 [+0 bytes / +0.00%]
code_size/hello_webgl_wasm.json: 12660 => 12660 [+0 bytes / +0.00%]
code_size/hello_webgl_wasm2js.json: 17947 => 17947 [+0 bytes / +0.00%]
code_size/random_printf_wasm.json: 12494 => 12494 [+0 bytes / +0.00%]
code_size/random_printf_wasm2js.json: 17262 => 17262 [+0 bytes / +0.00%]
other/codesize/test_codesize_cxx_ctors1.gzsize: 8219 => 8244 [+25 bytes / +0.30%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8207 => 8232 [+25 bytes / +0.30%]
other/codesize/test_codesize_cxx_except.gzsize: 9217 => 9239 [+22 bytes / +0.24%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8165 => 8190 [+25 bytes / +0.31%]
other/codesize/test_codesize_cxx_except_wasm_legacy.gzsize: 8165 => 8190 [+25 bytes / +0.31%]
other/codesize/test_codesize_cxx_lto.gzsize: 8234 => 8258 [+24 bytes / +0.29%]
other/codesize/test_codesize_cxx_mangle.gzsize: 9255 => 9279 [+24 bytes / +0.26%]
other/codesize/test_codesize_cxx_noexcept.gzsize: 8219 => 8244 [+25 bytes / +0.30%]
other/codesize/test_codesize_cxx_wasmfs.gzsize: 3440 => 3444 [+4 bytes / +0.12%]
other/codesize/test_codesize_files_js_fs.gzsize: 7528 => 7544 [+16 bytes / +0.21%]
other/codesize/test_codesize_files_wasmfs.gzsize: 2682 => 2685 [+3 bytes / +0.11%]
other/codesize/test_codesize_hello_O0.gzsize: 7940 => 7958 [+18 bytes / +0.23%]
other/codesize/test_codesize_hello_O1.gzsize: 2547 => 2548 [+1 bytes / +0.04%]
other/codesize/test_codesize_hello_dylink.gzsize: 5860 => 5866 [+6 bytes / +0.10%]
other/codesize/test_codesize_hello_single_file.gzsize: 3690 => 3693 [+3 bytes / +0.08%]
other/codesize/test_codesize_minimal_O0.gzsize: 6284 => 6296 [+12 bytes / +0.19%]
other/codesize/test_codesize_minimal_pthreads.gzsize: 4037 => 4042 [+5 bytes / +0.12%]

Average change: +0.14% (+0.00% - +0.31%)
```
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the fix!

LGTM % comment.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 27, 2025

If you merge / rebase with main the test_emcc_4 failure is now fixed.

@sbc100 sbc100 merged commit 0febee8 into emscripten-core:main Mar 27, 2025
24 checks passed
@kleisauke kleisauke deleted the mv-sharedModules-to-lib branch March 28, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants