Implement precise pthread wakeups#26659
Conversation
61f016e to
20d6937
Compare
|
"implemenat" => "implements" in description. |
This change implements the design in docs/design/01-precise-futex-wakeups.md Note: As part of this change the definition of emscripten_futex_wait was changes to explicitly allow spurious wakeups. Fixes: emscripten-core#26633
cb1c872 to
8dc3c30
Compare
|
|
||
| // If the given memory address contains value val, puts the calling thread to | ||
| // sleep waiting for that address to be notified. | ||
| // Note: Like the Linux futex syscall, this APi *does* allow spurious wakeups. |
There was a problem hiding this comment.
| // Note: Like the Linux futex syscall, this APi *does* allow spurious wakeups. | |
| // Note: Like the Linux futex syscall, this API *does* allow spurious wakeups. |
| // Note: Like the Linux futex syscall, this APi *does* allow spurious wakeups. | ||
| // This differs from the WebAssembly `atomic.wait` instruction itself which | ||
| // does *not* allow supurious wakeups and it means that most callers will want | ||
| // to wraps this some kind loop. |
There was a problem hiding this comment.
| // to wraps this some kind loop. | |
| // to wrap this in some kind loop. |
| return 0; | ||
| } | ||
| #ifdef __EMSCRIPTEN__ | ||
| // Wake the target thread in case it in emscripten_futex_wait. Normally |
There was a problem hiding this comment.
| // Wake the target thread in case it in emscripten_futex_wait. Normally | |
| // Wake the target thread in case it is in emscripten_futex_wait. Normally |
| } else { | ||
| DBG("emscripten_futex_wait skipping atomic.wait due to NOTIFY_BIT"); | ||
| // CAS failed, NOTIFY_BIT must have been set. In this case we don't | ||
| // actually wait at all. Instead we behaviour as if we spuriously woke up |
There was a problem hiding this comment.
| // actually wait at all. Instead we behaviour as if we spuriously woke up | |
| // actually wait at all. Instead we behave as if we spuriously woke up |
| return -ECANCELED; | ||
| } | ||
| #else | ||
| ret = __builtin_wasm_memory_atomic_wait32((int*)addr, val, max_wait_ns); |
There was a problem hiding this comment.
It would be good to add an // __EMSCRIPTEN_PTHREADS__ comment to the above #else. Otherwise the presence of a second wait here is very confusing. Actually, why do we wait when pthreads are not enabled? Is it just in case user code will handle the notify, e.g. from JS? Or is this a Wasm Workers thing?
| // Then proxying work to the main thread using the system queue we have | ||
| // a very special case in that we need the target thread to wake up from | ||
| // `emscripten_futex_wait` to process the queue. | ||
| if (rtn && is_system_queue && pthread_equal(target_thread, emscripten_main_runtime_thread_id())) { |
There was a problem hiding this comment.
It would be good to clang-format this to wrap to 80 columns. At least that's what the rest of the proxying code uses.
|
|
||
| return em_task_queue_send(tasks, t); | ||
| bool rtn = em_task_queue_send(tasks, t); | ||
| // Then proxying work to the main thread using the system queue we have |
There was a problem hiding this comment.
| // Then proxying work to the main thread using the system queue we have | |
| // When proxying work to the main thread using the system queue we have |
| } | ||
|
|
||
| return em_task_queue_send(tasks, t); | ||
| bool rtn = em_task_queue_send(tasks, t); |
There was a problem hiding this comment.
I would have expected ret rather than rtn. Unless there is common precedent for rtn in the code base?
| bool rtn = em_task_queue_send(tasks, t); | |
| bool ret = em_task_queue_send(tasks, t); |
| // since blocking is not allowed there). | ||
| int _emscripten_thread_supports_atomics_wait(void); | ||
|
|
||
| // Notify a thread that it has new work to do (cancellation or mailbox). |
There was a problem hiding this comment.
| // Notify a thread that it has new work to do (cancellation or mailbox). |
All the method does is inject the wakeup, so I would avoid suggesting that it also directly causes the target thread to do anything like check a mailbox.
|
|
||
| 5.0.6 (in development) | ||
| ---------------------- | ||
| - The emscripten_futux_wait API is not document to explicitly allow spurious |
There was a problem hiding this comment.
| - The emscripten_futux_wait API is not document to explicitly allow spurious | |
| - The emscripten_futux_wait API is now documented to explicitly allow spurious |
This change implements the design in docs/design/01-precise-futex-wakeups.md
As part of this change emscripten_futex_wait API now explicitly allows spurious wakeups, much like the corresponding linux syscall. There were a couple of tests that were using
emscripten_futex_waitwithout a loop, so I added loops to them in an abundance of caution.Fixes: #26633