[Proxying] Use Atomics.waitAsync to receive proxying notifications by tlively · Pull Request #18861 · emscripten-core/emscripten

tlively · 2023-02-27T18:23:47Z

When it is available, register to be notified of new proxying messages using
Atomics.waitAsync and perform the notification using Atomics.notify. This
new notification mechanism does not depend on the main thread forwarding
postMessage notifications, so its performance should not degrade when the main
thread is busy.

Update a test that depended on intercepting postMessage notifications to
conservatively wait for 20ms instead.

tlively · 2023-02-27T18:24:03Z

Current dependencies on/for this PR:

main
- PR [Proxying][NFC] Move deferred cleanup logic to em_task_queue #18851
  - PR [Proxying] Send messages via in-memory mailbox queues #18852
    - PR [Proxying] Cancel proxied work when the worker thread dies #18741
      - PR [Proxying] Update proxying with callback to support cancellation #18776
        
        PR [Proxying] Add emscripten_proxy_callback_with_ctx #18810
        
        PR [Proxying] Add proxying functions that return promises #18825
        
        PR [Proxying] Use Atomics.waitAsync to receive proxying notifications #18861 👈
        
        PR Remove unnecessary EXIT_RUNTIME setting from proxying tests #18873

This comment was auto-generated by Graphite.

tlively · 2023-02-27T21:14:51Z

The wasm2js test failures should be addressed by WebAssembly/binaryen#5525.

tlively · 2023-02-27T23:23:40Z

This dramatically improves performance for WasmFS/OPFS workloads that otherwise have low contention, even when the main thread isn't doing any extra work 🎉 🎉 🎉

Threads	OPFS w/ waitAsync (ms)	OPFS w/ postMessage (ms)	Ratio
1	320.7	508.8	0.63
2	157.2	304.5	0.52
4	142.6	169.4	0.84
8	123	129.1	0.95
16	141.5	148.7	0.95

(Measuring time to perform 10k fstat calls split across n threads, where each thread is working with its own separate file. Average of 10 runs with each thread count after 5 warmup runs. code)

When it is available, register to be notified of new proxying messages using `Atomics.waitAsync` and perform the notification using `Atomics.notify`. This new notification mechanism does not depend on the main thread forwarding postMessage notifications, so its performance should not degrade when the main thread is busy. Update a test that depended on intercepting postMessage notifications to conservatively wait for 20ms instead.

tlively · 2023-03-06T22:32:05Z

@sbc100, this is ready for a review!

sbc100 · 2023-03-07T18:04:08Z

    '__emscripten_tls_init',
    '_pthread_self',
    'checkMailbox',
+    '__emscripten_thread_mailbox_await',


Move this up to a few lines do it is grouped with the other __emscripten_thread_.. functions?

sbc100 · 2023-03-07T18:05:56Z

@@ -1 +1 @@
-15320
+15563


Presumable these losses could be more than regained if/when we stop including the old postMessage mechanism?

I guess we should add an option for that at some point? Perhaps doing it automatically on browser version?

Right now it's possible to proxy a message to a worker before it has called Atomics.waitAsync to start listening for notifications, so we still have to use the old code path occasionally even on browsers that support Atomics.waitAsync. We could fix that in principle, but it would require waiting for the new thread to execute Atomics.waitAsync before returning from pthread_create, so it would at least moving thread lifetime management off the main thread.

sbc100 · 2023-03-07T18:09:43Z


+      // Await mailbox notifications with `Atomics.waitAsync` so we can start
+      // using the fast `Atomics.notify` notification path.
+      Module['__emscripten_thread_mailbox_await'](e.data.pthread_ptr);


Should we just make this function always operate on the calling thread? Saved a couple of bytes here but not passing this argument?

Oh I see that this is implemented in JS which doesn't have easy direct access to the self pointer so maybe this makes sense actually.

Yeah, this argument avoids having to call back into Wasm just to get the current pthread pointer.

sbc100 · 2023-03-07T18:13:10Z

+  _emscripten_thread_mailbox_await__deps: ['$checkMailbox'],
+  _emscripten_thread_mailbox_await__sig: 'vp',
+  _emscripten_thread_mailbox_await: function(pthread_ptr) {
+    if (typeof Atomics.waitAsync === 'function') {


Should this be an assert instead? Are you supposed to be able to call this function at all on browsers that don't support this?

Right now we unconditionally call this function and handle the case where Atomics.waitAsync is not supported internally in this function. Handling that case in the caller would require determining whether Atomics.waitAsync is supported from inside the Wasm module, which would either require an additional import or a new global, so this way seems simpler.

sbc100 · 2023-03-07T18:14:32Z

+  _emscripten_thread_mailbox_await: function(pthread_ptr) {
+    if (typeof Atomics.waitAsync === 'function') {
+      // TODO: How to make this work with wasm64?
+      var wait = Atomics.waitAsync(HEAP32, pthread_ptr >> 2, pthread_ptr);


So the waiting is don't on the pthread pointer itself? (not some member?)

Yes. It doesn't really matter what address we wait on as long as we know what its value will be when we execute Atomics.waitAsync. The first member of the pthread struct is conveniently a pointer to itself, so we know what value it will have when we wait on it.

sbc100 · 2023-03-07T18:15:24Z

+#if ASSERTIONS
+      assert(wait.async);
+#endif
+      wait.value.then(() => checkMailbox());


Can you just write wait.value.then(checkMailbox)?

Ah yes. In a previous version of this code, checkMailbox took an optional argument, so this extra closure was necessary.

sbc100 · 2023-03-07T18:17:11Z

+    if (pthread_ptr) {
+      // If we are using Atomics.waitAsync as our notification mechanism, wait for
+      // a notification before processing the mailbox to avoid missing any work.
+      __emscripten_thread_mailbox_await(pthread_ptr);


This seems cyclical, since emscripten_thread_mailbox_await then triggers call to checkMailbox, no?

Yes, but only as a callback to a promise, so this doesn't cause an infinite recursion. This is how we set up for the next notification when we receive a first notification.

sbc100 · 2023-03-07T18:17:43Z

-                               pthread_self(),
-                               emscripten_main_runtime_thread_id());
+    if (thread->waiting_async) {
+      __builtin_wasm_memory_atomic_notify((int*)thread, -1);


So I guess this system doesn't actually write anything to the thread pointer here?

I see so we keep waiting in a loop for the next thing.

Correct. At no point does the first member of the pthread struct hold anything other than a pointer to itself.

…mscripten-core#18861) When it is available, register to be notified of new proxying messages using `Atomics.waitAsync` and perform the notification using `Atomics.notify`. This new notification mechanism does not depend on the main thread forwarding postMessage notifications, so its performance should not degrade when the main thread is busy. Update a test that depended on intercepting postMessage notifications to conservatively wait for 20ms instead.

This TODO was added in emscripten-core#18861 when the following line read: ``` Atomics.waitAsync(HEAP32, pthread_ptr >> 2, pthread_ptr); ``` I'm not sure if the TODO here was refering to that 2nd or 3rd argument but they both work fine under wasm64 right now. The second argument now uses `getHeapOffset('pthread_ptr', 'i32')` which works find under wasm64 even for addresses over 4gb. The third argument here is actually not important since its value never changes, it just needs to match the contents of the memory at the specific location. I verified that this works in wasm64_4gb mode. The `pthread_ptr` is simply converted to a U32 and then compared to the low bits of the 64-bit pthread_ptr in memory. The assert on the line below ensure that this does indeed work. If it didn't then `"not-equal"` would be returned from `Atomics.waitAsync`

This TODO was added in #18861 when the following line read: ``` Atomics.waitAsync(HEAP32, pthread_ptr >> 2, pthread_ptr); ``` I'm not sure if the TODO here was refering to that 2nd or 3rd argument but they both work fine under wasm64 right now. The second argument now uses `getHeapOffset('pthread_ptr', 'i32')` which works find under wasm64 even for addresses over 4gb. The third argument here is actually not important since its value never changes, it just needs to match the contents of the memory at the specific location. I verified that this works in wasm64_4gb mode. The `pthread_ptr` is simply converted to a U32 and then compared to the low bits of the 64-bit pthread_ptr in memory. The assert on the line below ensure that this does indeed work. If it didn't then `"not-equal"` would be returned from `Atomics.waitAsync` --------- Co-authored-by: Thomas Lively <tlively123@gmail.com>

tlively requested a review from sbc100 February 27, 2023 18:23

This was referenced Feb 27, 2023

[Proxying] Add emscripten_proxy_callback_with_ctx #18810

Merged

[Proxying] Update proxying with callback to support cancellation #18776

Merged

tlively force-pushed the proxying-promise branch from 37d81c9 to 5573a31 Compare February 28, 2023 15:39

tlively force-pushed the proxying-atomics-wait-async branch from f898a5d to b04b843 Compare February 28, 2023 15:39

tlively mentioned this pull request Feb 28, 2023

Remove unnecessary EXIT_RUNTIME setting from proxying tests #18873

Merged

tlively force-pushed the proxying-promise branch from 5573a31 to a2d0ef3 Compare March 1, 2023 01:10

tlively force-pushed the proxying-atomics-wait-async branch from b04b843 to bfa8d15 Compare March 1, 2023 01:10

tlively force-pushed the proxying-promise branch from a2d0ef3 to 0fa4fc7 Compare March 1, 2023 16:38

tlively force-pushed the proxying-atomics-wait-async branch from bfa8d15 to d44c1f9 Compare March 1, 2023 16:38

tlively force-pushed the proxying-promise branch from 0fa4fc7 to a8184bf Compare March 1, 2023 19:37

tlively force-pushed the proxying-atomics-wait-async branch from d44c1f9 to 1cf29d0 Compare March 1, 2023 19:37

tlively force-pushed the proxying-promise branch from a8184bf to c3b5047 Compare March 1, 2023 21:07

tlively force-pushed the proxying-atomics-wait-async branch from 1cf29d0 to bb28e08 Compare March 1, 2023 21:07

tlively force-pushed the proxying-promise branch from c3b5047 to c30ca68 Compare March 1, 2023 22:33

tlively force-pushed the proxying-atomics-wait-async branch from bb28e08 to afcb63b Compare March 1, 2023 22:33

tlively force-pushed the proxying-promise branch from c30ca68 to 2f1ddd5 Compare March 2, 2023 02:21

tlively force-pushed the proxying-atomics-wait-async branch 2 times, most recently from 9b5d06e to 1cd0572 Compare March 4, 2023 00:27

tlively force-pushed the proxying-promise branch from 0bc125b to c3c38e7 Compare March 6, 2023 16:46

tlively force-pushed the proxying-atomics-wait-async branch from 1cd0572 to 7b1ac4d Compare March 6, 2023 16:46

Base automatically changed from proxying-promise to main March 6, 2023 18:09

tlively added 2 commits March 6, 2023 11:50

update metadce tests

eb5fbd4

update heartbeat comment

02bebfe

tlively force-pushed the proxying-atomics-wait-async branch from 7b1ac4d to 02bebfe Compare March 6, 2023 19:53

sbc100 reviewed Mar 7, 2023

View reviewed changes

sbc100 approved these changes Mar 7, 2023

View reviewed changes

address feedback

0a051f2

tlively enabled auto-merge (squash) March 7, 2023 23:20

tlively merged commit db28eec into main Mar 8, 2023

tlively deleted the proxying-atomics-wait-async branch March 8, 2023 01:19

lambdageek mentioned this pull request Nov 15, 2023

[browser][MT] pool don't yield to event loop dotnet/runtime#94600

Closed

sbc100 mentioned this pull request Apr 1, 2026

Remove TODO from libpthread.js. NFC #26603

Merged

		@@ -1 +1 @@
		15320
		15563

Uh oh!

Conversation

tlively commented Feb 27, 2023

Uh oh!

tlively commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented Feb 27, 2023

Uh oh!

tlively commented Feb 27, 2023

Uh oh!

tlively commented Mar 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tlively commented Feb 27, 2023 •

edited

Loading