Skip to content

Move atomics.wait path for emscripten_futex_wait to native code. NFC #15742

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

Merged
merged 1 commit into from
Dec 12, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 10, 2021

Add a new WebAssembly global to the native side that signals if
the current thread can use atomics.wait and use this to implement
the atomics.wait version of emscripten_futex_wait.

The non-blocking/busy-looping path is still implemented in JS
for now. It could potentially be moved as a followup.

See #15061

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 10, 2021

View diff with "ignore whitespace"

@sbc100 sbc100 force-pushed the futex_wait_native branch 2 times, most recently from d676cee to 149b123 Compare December 10, 2021 19:25
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I'm happy to be able to use the Wasm instruction for waiting.

@sbc100 sbc100 enabled auto-merge (squash) December 10, 2021 21:06
sbc100 added a commit to WebAssembly/binaryen that referenced this pull request Dec 10, 2021
Also, fix bug where pointer was being used direcltly to
index into Int32Array.  I suppose this code had basically
zero users until I tried to land this change in emscripten:
emscripten-core/emscripten#15742
sbc100 added a commit to WebAssembly/binaryen that referenced this pull request Dec 11, 2021
Also, fix bug where pointer was being used direcltly to
index into Int32Array.  I suppose this code had basically
zero users until I tried to land this change in emscripten:
emscripten-core/emscripten#15742
Copy link
Collaborator

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I left a small question as an inline comment.

sbc100 added a commit to WebAssembly/binaryen that referenced this pull request Dec 11, 2021
Also, fix bug where pointer was being used direcltly to
index into Int32Array.  I suppose this code had basically
zero users until I tried to land this change in emscripten:
emscripten-core/emscripten#15742
@sbc100 sbc100 force-pushed the futex_wait_native branch 2 times, most recently from 2b3c092 to a30cc15 Compare December 12, 2021 18:43
@sbc100 sbc100 changed the title Add new emscripten_thread_can_block function and use it in emscripten_futex_wait. NFC Move atomics.wait path for emscripten_futex_wait to native code. NFC Dec 12, 2021
Add a new WebAssembly global to the native side that signals if
the current thread can use `atomics.wait` and use this to implement
the `atomics.wait` version of `emscripten_futex_wait`.

The non-blocking/busy-looping path is still implemented in JS
for now.  It could potentially be moved as a followup.
@sbc100 sbc100 disabled auto-merge December 12, 2021 21:33
@sbc100 sbc100 enabled auto-merge (squash) December 12, 2021 21:33
@sbc100 sbc100 merged commit ad05649 into main Dec 12, 2021
@sbc100 sbc100 deleted the futex_wait_native branch December 12, 2021 22:32
mmarczell-graphisoft pushed a commit to GRAPHISOFT/emscripten that referenced this pull request Jan 5, 2022
…NFC (emscripten-core#15742)

Add a new WebAssembly global to the native side that signals if
the current thread can use `atomics.wait` and use this to implement
the `atomics.wait` version of `emscripten_futex_wait`.

The non-blocking/busy-looping path is still implemented in JS
for now.  It could potentially be moved as a followup.
sbc100 added a commit that referenced this pull request Dec 19, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
sbc100 added a commit that referenced this pull request Dec 19, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
sbc100 added a commit that referenced this pull request Dec 19, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
sbc100 added a commit that referenced this pull request Dec 19, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
sbc100 added a commit that referenced this pull request Dec 20, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
sbc100 added a commit that referenced this pull request Dec 23, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
sbc100 added a commit that referenced this pull request Dec 26, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
sbc100 added a commit that referenced this pull request Dec 26, 2022
When I ported this code from JS back in #15742 I mistakenly changed
the semantics here.  This change restores the previous behaviour of
using atomic.wait except in on the main browser thread.

The `futex_wait_busy` can clearly only be used a single main thread and
is not appropriate for using in cases where there are other threads in
the system that also don't support `atomic.wait` (such as IIUC audio
worklets).  To support that case we would need a different busy wait
function that didn't depend on some global singleton like
_emscripten_main_thread_futex.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants