-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proxying] Use Atomics.waitAsync to receive proxying notifications #18861
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1219,18 +1219,40 @@ var LibraryPThread = { | |
}, | ||
#endif // MAIN_MODULE | ||
|
||
$checkMailbox__deps: ['$callUserCallback'], | ||
$checkMailbox__deps: ['$callUserCallback', | ||
'_emscripten_thread_mailbox_await'], | ||
$checkMailbox: function() { | ||
// Only check the mailbox if we have a live pthread runtime. We implement | ||
// pthread_self to return 0 if there is no live runtime. | ||
if (_pthread_self()) { | ||
var pthread_ptr = _pthread_self(); | ||
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); | ||
callUserCallback(() => __emscripten_check_mailbox()); | ||
} | ||
}, | ||
|
||
_emscripten_notify_mailbox__deps: ['$checkMailbox'], | ||
_emscripten_notify_mailbox__sig: 'vppp', | ||
_emscripten_notify_mailbox: function(targetThreadId, currThreadId, mainThreadId) { | ||
_emscripten_thread_mailbox_await__deps: ['$checkMailbox'], | ||
_emscripten_thread_mailbox_await__sig: 'vp', | ||
_emscripten_thread_mailbox_await: function(pthread_ptr) { | ||
if (typeof Atomics.waitAsync === 'function') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now we unconditionally call this function and handle the case where |
||
// TODO: How to make this work with wasm64? | ||
var wait = Atomics.waitAsync(HEAP32, pthread_ptr >> 2, pthread_ptr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the waiting is don't on the pthread pointer itself? (not some member?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. It doesn't really matter what address we wait on as long as we know what its value will be when we execute |
||
#if ASSERTIONS | ||
assert(wait.async); | ||
#endif | ||
wait.value.then(checkMailbox); | ||
var waitingAsync = pthread_ptr + {{{ C_STRUCTS.pthread.waiting_async }}}; | ||
Atomics.store(HEAP32, waitingAsync >> 2, 1); | ||
} | ||
}, | ||
|
||
_emscripten_notify_mailbox_postmessage__deps: ['$checkMailbox'], | ||
_emscripten_notify_mailbox_postmessage__sig: 'vppp', | ||
_emscripten_notify_mailbox_postmessage: function(targetThreadId, | ||
currThreadId, | ||
mainThreadId) { | ||
if (targetThreadId == currThreadId) { | ||
setTimeout(() => checkMailbox()); | ||
} else if (ENVIRONMENT_IS_PTHREAD) { | ||
|
@@ -1241,7 +1263,7 @@ var LibraryPThread = { | |
#if ASSERTIONS | ||
err('Cannot send message to thread with ID ' + targetThreadId + ', unknown thread ID!'); | ||
#endif | ||
return /*0*/; | ||
return; | ||
} | ||
worker.postMessage({'cmd' : 'checkMailbox'}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,6 +216,10 @@ function handleMessage(e) { | |
// Pass the thread address to wasm to store it for fast access. | ||
Module['__emscripten_thread_init'](e.data.pthread_ptr, /*isMainBrowserThread=*/0, /*isMainRuntimeThread=*/0, /*canBlock=*/1); | ||
|
||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this argument avoids having to call back into Wasm just to get the current pthread pointer. |
||
|
||
#if ASSERTIONS | ||
assert(e.data.pthread_ptr); | ||
#endif | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,7 @@ void _emscripten_thread_mailbox_shutdown(pthread_t thread) { | |
void _emscripten_thread_mailbox_init(pthread_t thread) { | ||
thread->mailbox = em_task_queue_create(thread); | ||
thread->mailbox_refcount = 1; | ||
thread->waiting_async = 0; | ||
} | ||
|
||
// Exported for use in worker.js, but otherwise an internal function. | ||
|
@@ -92,9 +93,9 @@ void _emscripten_check_mailbox() { | |
// Send a postMessage notification telling the target thread to check its | ||
// mailbox when it returns to its event loop. Pass in the current thread and | ||
// main thread ids to minimize calls back into Wasm. | ||
void _emscripten_notify_mailbox(pthread_t target_thread, | ||
pthread_t curr_thread, | ||
pthread_t main_thread); | ||
void _emscripten_notify_mailbox_postmessage(pthread_t target_thread, | ||
pthread_t curr_thread, | ||
pthread_t main_thread); | ||
|
||
void emscripten_thread_mailbox_send(pthread_t thread, task t) { | ||
assert(thread->mailbox_refcount > 0); | ||
|
@@ -112,8 +113,11 @@ void emscripten_thread_mailbox_send(pthread_t thread, task t) { | |
notification_state previous = | ||
atomic_exchange(&thread->mailbox->notification, NOTIFICATION_PENDING); | ||
if (previous != NOTIFICATION_PENDING) { | ||
_emscripten_notify_mailbox(thread, | ||
pthread_self(), | ||
emscripten_main_runtime_thread_id()); | ||
if (thread->waiting_async) { | ||
__builtin_wasm_memory_atomic_notify((int*)thread, -1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I guess this system doesn't actually write anything to the thread pointer here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see so we keep waiting in a loop for the next thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. At no point does the first member of the pthread struct hold anything other than a pointer to itself. |
||
} else { | ||
_emscripten_notify_mailbox_postmessage( | ||
thread, pthread_self(), emscripten_main_runtime_thread_id()); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ B | |
C | ||
D | ||
E | ||
o | ||
F | ||
p | ||
q | ||
r | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,3 +12,4 @@ a.k | |
a.l | ||
a.m | ||
a.n | ||
a.o |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
15320 | ||
15563 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now it's possible to proxy a message to a worker before it has called |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,3 +12,4 @@ k | |
l | ||
m | ||
n | ||
o |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
18922 | ||
18954 |
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.
This seems cyclical, since emscripten_thread_mailbox_await then triggers call to
checkMailbox
, no?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.
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.