Skip to content

Conversation

@Master-Hash
Copy link
Contributor

@Master-Hash Master-Hash commented Jul 16, 2025

Currently, waitForMsgType(self, 'wasm_bindgen_worker_init').then() will run on both main thread and worker thread. However, the main thread don't need to wait wasm_bindgen_worker_init, no such message will come in, the event listener will never be removed, resulting in memory leak.

I add the condition to make waitForMsgType(self, 'wasm_bindgen_worker_init').then() only runs on worker thread.

I've tested it on my real world application bundled with vite. No bundled version is not tested, but I believe it will work.

@RReverser
Copy link
Owner

Please provide more description. Why do you want this / what issue is this PR fixing?

@Master-Hash
Copy link
Contributor Author

I edited the description. Sorry for that, I thought it was self explainable.

@RReverser
Copy link
Owner

RReverser commented Jul 16, 2025

However, the main thread don't need to wait wasm_bindgen_worker_init, no such message will come in, the event listener will never be removed, resulting in memory leak.

I wouldn't really count this as memory leak, at least no more than any other statically-initialized stuff that is allocated only once at startup (including JS module, Wasm itself, Workers, etc).

Aside from slight increase in complexity, this will actually break a real-world scenario where someone can be loading the main JS itself in their own Web Worker.

This is useful when you're not interacting with DOM directly, and, in fact, until recently has been the official recommendation for loading wasm-bindgen-rayon to work around limitations related to blocking the main thread on the Web.

While we don't recommend that anymore and have an internal workaround that "just works" via spin-lock instead, loading the main JS in own Worker is still a useful optimisation in many scenarios.

If this PR is mainly meant as an optimisation and not fixing a bug you ran into, I'd rather keep things as-is.

@RReverser
Copy link
Owner

Although... Thinking a bit more, I guess wasm_bindgen_worker_init wouldn't break even in that scenario.

Still, I'd rather not add the extra logic unless it solves a demonstrable bug.

@Master-Hash
Copy link
Contributor Author

I think:

  1. In case the workerHelpers.js runs on main thread: As I described.
  2. In case the workerHelpers.js runs on another worker thread: the condition is always true, nothing change.

@RReverser
Copy link
Owner

FWIW the way we do this in Emscripten thread support is by having special name for Workers. If you want, you can do that, it should simplify the implementation a bit and achieve the desired outcome a bit more precisely as well.

@Master-Hash Master-Hash force-pushed the main-thread-no-wasm_bindgen_worker_init branch 4 times, most recently from cef539a to eca4f87 Compare July 16, 2025 14:22
@Master-Hash
Copy link
Contributor Author

Still, I'd rather not add the extra logic unless it solves a demonstrable bug.

I can maintain a fork.


From my perspective: When I was to understand which part of code runs on thread-builder thread or worker thread, that part made me confused. I think extra condition can reduce such confusion.

@Master-Hash Master-Hash force-pushed the main-thread-no-wasm_bindgen_worker_init branch from eca4f87 to 10ed5a6 Compare July 17, 2025 04:24
@RReverser RReverser merged commit 6b851de into RReverser:main Jul 17, 2025
1 check passed
@Master-Hash Master-Hash deleted the main-thread-no-wasm_bindgen_worker_init branch July 17, 2025 15:33
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