dylink: Share dynamicLibraries array across workers
#19496
Conversation
|
The idea with removing this was that pthreads all will call However, maybe that isn't working for |
test/test_core.py
Outdated
| self.emcc_args += ['--pre-js', 'pre.js'] | ||
| self.emcc_args += ['--extern-post-js=extern-post.js'] | ||
| self.set_setting('MODULARIZE') | ||
| self.set_setting('EXPORT_NAME=ModuleFactory') |
There was a problem hiding this comment.
I don't think we need these two settings to very that dynamicLibraries is shared do we?
|
This is something that's hitting Godot engine builds as well (filed as #20507, related issue in godot repo: godotengine/godot#82865). I tested an emscripten-releases build from today with the changes from this PR applied and that resolves the issue. |
Hmm, I wasn't aware of this. I thought it was controlled by Line 969 in 054bf30 Note that I also build with |
test/test_core.py
Outdated
| @@ -9510,6 +9514,9 @@ def test_Module_dynamicLibraries(self, args): | |||
| err('sharedModules: ' + Object.keys(sharedModules)); | |||
| assert('liblib.so' in sharedModules); | |||
| assert(sharedModules['liblib.so'] instanceof WebAssembly.Module); | |||
|
|
|||
| // Verify whether the main thread passes Module.dynamicLibraries to the worker | |||
| assert(Module['dynamicLibraries'].includes('liblib.so')); | |||
There was a problem hiding this comment.
Can you find a way to make a test case that fails, rather than verifying the contents dynamicLibraries. My understanding is that _emscripten_dlsync_self should be enough to have each new thread be in sync with the main thread, so there should be no need for dynamicLibraries on worker threads.
There was a problem hiding this comment.
i.e. can you make a test case that fails with onmessage() captured an uncaught exception: RangeError: WebAssembly.Table.get(): invalid index [index] into funcref table of size [size] on main?
There was a problem hiding this comment.
Sure, see commit b1837ef. Though, that fails on main with Maximum call stack size exceeded, so not exactly the same error.
FWIW, in the project I'm currently working on, the error manifests as null function or function signature mismatch when trying to use a function available in the side module.
|
@kleisauke Is your PR ready to be merged? Did you investigate why it fails CI wise? I'm the Web lead of the Godot Engine, and I just found out about this issue, as I somehow missed godotengine/godot#82865. We're kinda stuck to use 3.1.39 for our 4.3 release. I'll try to fork emscripten to test out your PR, if I can be of any help. |
|
@adamscott This PR should be ready, except that I need to rebase it to resolve the conflicts. The failed CI tests seem unrelated to this PR. FWIW, I opened a separate reproducer in #20625. wasm-vips has been cherry-picking this PR in a custom patchset for a while now: 3.1.61...kleisauke:wasm-vips-3.1.61. |
To ensure that the same dynamic libraries are loaded among all workers, it remains necessary to share the `dynamicLibraries` array, especially when it is defined outside the module (e.g. in `MODULARIZE` mode). Fixes a regression introduced in commit ceb2e28.
kleisauke
force-pushed
the
fix-dynamicLibraries-regression
branch
from
b1837ef
to
005a033
Compare
|
I confirm that this PR works. I was able to build a Godot 4.3 Web template (with threads and dlink enabled) using this PR (rebased @ 3.1.62-git) and export a game with it without any issues.
|
To ensure that the same dynamic libraries are loaded among all workers, it remains necessary to share the
dynamicLibrariesarray, especially when it is defined outside the module (e.g. inMODULARIZEmode).Fixes a regression introduced in commit ceb2e28.
Resolves: #20507.