[skwasm] Dynamic Threading #164748
[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 in Discord (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 the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
engine
|
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.
I see randomly missing spaces, e.g. sometimes it's a = b but sometimes a=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.
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 and const 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.
Looks like instance can be const.
| console.log("adding queuing listener"); | ||
| addEventListener("message", eventListener); | ||
| }; | ||
| console.log("adding initial listener"); |
There was a problem hiding this comment.
I'm not sure how useful the console.logs are to the user. This will generate a lot of noise.
There was a problem hiding this comment.
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.
Hmm, how does this work? Line 22 asserts emscripten_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.
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.
The emscripten_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 a postMessage. 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.
eyebrowsoffire
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
This is a reland of flutter#164748, which was reverted here: flutter#165350 It was reverted due to some issues in the flutter packages roller: flutter#165347 The unit test in flutter/packages was modified to be more deterministic and less affected by minor timing differences: flutter/packages#8920 So 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: #165350 It was reverted due to some issues in the flutter packages roller: #165347 The unit test in flutter/packages was modified to be more deterministic and less affected by minor timing differences: flutter/packages#8920 So this is basically being relanded without any significant changes, since the downstream tests have been fixed.
This is a reland of flutter#164748, which was reverted here: flutter#165350 It was reverted due to some issues in the flutter packages roller: flutter#165347 The unit test in flutter/packages was modified to be more deterministic and less affected by minor timing differences: flutter/packages#8920 So this is basically being relanded without any significant changes, since the downstream tests have been fixed.
This is a reland of flutter#164748, which was reverted here: flutter#165350 It was reverted due to some issues in the flutter packages roller: flutter#165347 The unit test in flutter/packages was modified to be more deterministic and less affected by minor timing differences: flutter/packages#8920 So 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.
This reverts commit b2a4a05. This has been causing issues when rolling to flutter/packages repo. See flutter#165347.
This is a reland of flutter#164748, which was reverted here: flutter#165350 It was reverted due to some issues in the flutter packages roller: flutter#165347 The unit test in flutter/packages was modified to be more deterministic and less affected by minor timing differences: flutter/packages#8920 So this is basically being relanded without any significant changes, since the downstream tests have been fixed.
3 participants
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.