- Notifications
You must be signed in to change notification settings - Fork30k
Comments
[skwasm] Dynamic Threading#164748
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel inDiscord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself,is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, soall reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
eyebrowsoffire commentedMar 7, 2025
This change doesn't require additional unit tests, because there is no change in functionality. The existing unit tests should be sufficient to confirm that we didn't break any existing functionality with this change. |
| const url = resolveUrlWithSegments(baseUrl, filename); | ||
| return URL.createObjectURL(new Blob( | ||
| [` | ||
| "use strict"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see randomly missing spaces, e.g. sometimes it'sa = b but sometimesa=b, or) => { vs)=>{, etc. Is that to save over-the-wire payload size, or just hand-formatting errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I took the original file generated by emscripten and modified it until I got it working properly, and then forgot to go back and clean it up (remove debug print statements, use consistent spacing andconst and stuff). So I need to go back and clean this up.
| const pendingMessages = []; | ||
| const d = message.data; | ||
| d["instantiateWasm"] = (info,receiveInstance) => { | ||
| var instance=new WebAssembly.Instance(d["wasm"],info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks likeinstance can beconst.
| console.log("adding queuing listener"); | ||
| addEventListener("message", eventListener); | ||
| }; | ||
| console.log("adding initial listener"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure how useful theconsole.logs are to the user. This will generate a lot of noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should avoid noise here except in exceptional cases – or for time-bound debugging on our side.
| assert(emscripten_is_main_browser_thread()); | ||
| _thread = emscripten_malloc_wasm_worker(65536); | ||
| emscripten_wasm_worker_post_function_v(_thread, []() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm, how does this work? Line 22 assertsemscripten_is_main_browser_thread(), which means the worker doesn't execute this code. If so, then the worker doesn't have this closure that's supposed to connect to the main thread. Is this some emscripten magic? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I can break down how this works. So this C++ lambda is not actually a closure, since it doesn't actually close over any variables (there are no captures). As such, a stateless lambda with no closures is special-cased in C++, and it decays to a normal vanilla C function pointer. So if this lambda did capture any variables, this wouldn't work at all, the lambda wouldn't be able to decay to a C function pointer. But with no captures, you can essentially think of the compiler just treating it as a scoped static C function.
Theemscripten_wasm_worker_post_function_v takes a pointer to a C function as an argument. Whenever the address of a function is taken, emscripten/llvm know to add that function to the global function table, and the pointer to that C function is essentially just an index into that global function table. The emscripten wasm workers runtime has JS support code that basically takes this function index and ships it across to the worker via apostMessage. When the worker processes this message, it looks up the function in its own global function table by index, and the global function tables should be identical between the instance on the main thread and the instance on the worker.
b2a4a05Uh oh!
There was an error while loading.Please reload this page.
This is a reland offlutter#164748, which wasreverted here:flutter#165350It was reverted due to some issues in the flutter packages roller:flutter#165347The unit test in flutter/packages was modified to be more deterministic and less affected by minor timing differences:flutter/packages#8920So this is basically being relanded without any significant changes, since the downstream tests have been fixed.
This is a reland of#164748,which was reverted here:#165350It was reverted due to some issues in the flutter packages roller:#165347The unit test in flutter/packages was modified to be more deterministicand less affected by minor timing differences:flutter/packages#8920So this is basically being relanded without any significant changes,since the downstream tests have been fixed.
This is a reland offlutter#164748, which wasreverted here:flutter#165350It was reverted due to some issues in the flutter packages roller:flutter#165347The unit test in flutter/packages was modified to be more deterministic and less affected by minor timing differences:flutter/packages#8920So this is basically being relanded without any significant changes, since the downstream tests have been fixed.
This is a reland offlutter#164748,which was reverted here:flutter#165350It was reverted due to some issues in the flutter packages roller:flutter#165347The unit test in flutter/packages was modified to be more deterministicand less affected by minor timing differences:flutter/packages#8920So this is basically being relanded without any significant changes,since the downstream tests have been fixed.
This switches skwasm over from the emscripten pthreads implementation toemscripten's "wasm workers" threading implementation. The pthreadsimplementation simply will not run at all in a non-crossOriginIsolatedcontext, but the wasm workers implementation only fails if we actuallyattempt to spawn a thread. This means we can actually choose whether touse a single-threaded or multi-threaded strategy at runtime, which meanswe don't have to build two variants of skwasm for single- vsmulti-threaded.
This reverts commitb2a4a05.This has been causing issues when rolling to flutter/packages repo. Seeflutter#165347.
This is a reland offlutter#164748,which was reverted here:flutter#165350It was reverted due to some issues in the flutter packages roller:flutter#165347The unit test in flutter/packages was modified to be more deterministicand less affected by minor timing differences:flutter/packages#8920So this is basically being relanded without any significant changes,since the downstream tests have been fixed.
This switches skwasm over from the emscripten pthreads implementation to emscripten's "wasm workers" threading implementation. The pthreads implementation simply will not run at all in a non-crossOriginIsolated context, but the wasm workers implementation only fails if we actually attempt to spawn a thread. This means we can actually choose whether to use a single-threaded or multi-threaded strategy at runtime, which means we don't have to build two variants of skwasm for single- vs multi-threaded.