Move `emscripten_atomic_wait_async` to atomic.h and add core testing #20404

sbc100 · 2023-10-05T22:09:54Z

This API was orininally added for wasm workers but is not wasm worker specific.

As part of making this API compatible with pthreads I added support for
runtimeKeepalivePush/Pop and callUserCallback which means that this PR
subsumes #20306.

Followup to #20381.

src/library_atomic.js

juj · 2023-10-16T13:15:18Z

src/library_atomic.js

+  $polyfillWaitAsync: () => {
+    // nop, used for its postset to ensure waitAsync() polyfill is included
+    // exaclyt once.
+    // Any function using Atomics.waitAsync should depend on this.


Hmm, I suppose you are looking to fix a situation where only JS code would ever be doing Atomics.waitAsync()s, and Wasm code is doing all the wakes, hence the polyfill would never be emitted?

This situation looks like something that would be good to go in the Atomics related documentation?

Reviewing this PR for these functional changes is a bit tricky, given the code move. Are there other functional differences?

Hmm, I suppose you are looking to fix a situation where only JS code would ever be doing Atomics.waitAsync()s, and Wasm code is doing all the wakes, hence the polyfill would never be emitted?

This situation looks like something that would be good to go in the Atomics related documentation?

I'm not quite sure I understand. Atomics.waitAsync can only ever be performed by JS code, since its JS only function, right? There is no wasm version of it.

This dependency just means the polyfill is only ever included when the library functions that call it are included. Isn't that desirable? Or would you rather the polyfill always be included?

Reviewing this PR for these functional changes is a bit tricky, given the code move. Are there other functional differences?

I'm pretty sure I didn't change anything functionally on the JS library side, just moved stuff around.

Hmm, I suppose you are looking to fix a situation where only JS code would ever be doing Atomics.waitAsync()s, and Wasm code is doing all the wakes, hence the polyfill would never be emitted?

This situation looks like something that would be good to go in the Atomics related documentation?

I can split out the __deps fix and land that first to make it more obvious what is going on here ..

There is one change to the JS code here, in order to support running emscripten_atomic_wait_async under node: I added runtimeKeepalivePush/runtimeKeepalivePop and callUserCallback to emscripten_atomic_wait_async. This means that the runtime will not exit will async waits are running, but will exit cleanly once the last one is done.

Split out from emscripten-core#20404. Rather than adding artificial dependencies on `emscripten_atomic_wait_async` we can use the same technique used by `$polyfillSetImmediate` in `library_eventloop.js`.

Split out from #20404. Rather than adding artificial dependencies on `emscripten_atomic_wait_async` we can use the same technique used by `$polyfillSetImmediate` in `library_eventloop.js`.

sbc100 · 2023-10-19T16:41:45Z

Rebased on top of #20474

kripken

Reviewing the changes since my last lgtm, all the changed files lgtm aside from src/library_atomic.js which has changes and is very large. Any chance you have a diff compared to my last review?

I think adding commits on top of an existing PR is really better for this workflow, as I've said in the past. For small stuff it doesn't matter, but there's 150 lines of code in that file here.

sbc100 · 2023-10-19T18:12:19Z

Reviewing the changes since my last lgtm, all the changed files lgtm aside from src/library_atomic.js which has changes and is very large. Any chance you have a diff compared to my last review?

I think adding commits on top of an existing PR is really better for this workflow, as I've said in the past. For small stuff it doesn't matter, but there's 150 lines of code in that file here.

Nothing should have changed here since that last review, just rebased on top of #20474

sbc100 · 2023-10-19T18:15:21Z

I think adding commits on top of an existing PR is really better for this workflow, as I've said in the past. For small stuff it doesn't matter, but there's 150 lines of code in that file here.

The reason that is hard for me to do that since my workflow is basically a rebase one. I use git rebase-update to rebase all my active branches (normally > 30 at any given time). I suppose could have a list of branches that I opt out of the rebase flow and use a merge flow instead for them but I fear it would add a fair amount of overhead, and I'm not sure I how I would decide for a given PR if i use switch to the merge flow (which I am less familiar with).

How about this: The next time you see a PR from me that you think you would rather see review in a merge workflow can you comment there and I will give it a try.

kripken · 2023-10-19T18:20:45Z

Rebasing sgtm - it's the squashing that is the issue for me, iiuc.

That is, if this PR had one commit, then you realized the need for a fix so you added another commit, and then you rebased them both on main, that would be fine: I'd review the second commit in detail (I'd still need to look at the first commit, so it's not as good as a merge-based workflow, but I can live with that).

Is adding more commits as you go tricky for some reason? Does the rebase workflow always squash them for you for some reason?

sbc100 · 2023-10-19T18:28:11Z

Rebasing sgtm - it's the squashing that is the issue for me, iiuc.

That is, if this PR had one commit, then you realized the need for a fix so you added another commit, and then you rebased them both on main, that would be fine: I'd review the second commit in detail (I'd still need to look at the first commit, so it's not as good as a merge-based workflow, but I can live with that).

Is adding more commits as you go tricky for some reason? Does the rebase workflow always squash them for you for some reason?

Ah no, you are right, its the rebasing that is kind of key to my workflow, not the constant squashing. I could avoid that in some cases.

However, IIRC github still gets confused about incremental reviewing when rebasing happens, even if squashing doesn't happen. You can still review each commit if you like, but githubs "show be whats changed since I last looked" feature I think doesn't work.

kripken · 2023-10-19T20:05:45Z

At least in this PR "show me what changed" seems to have worked. Some files were marked as unchanged and some were marked as changed. (I didn't have a way to verify that it was the right ones, though.)

This API was orininally added for wasm workers but is not wasm worker specific. Followup to emscripten-core#20381.

sbc100 requested review from juj, tlively and kripken October 5, 2023 22:09

sbc100 force-pushed the move_wait_async branch 3 times, most recently from 57091df to 3904f1c Compare October 5, 2023 22:40

sbc100 mentioned this pull request Oct 5, 2023

Fix async futex wait in WASM worker dose not keep runtime alive #20306

Closed

kripken approved these changes Oct 5, 2023

View reviewed changes

sbc100 force-pushed the move_wait_async branch 9 times, most recently from acbdf92 to 1fe2c44 Compare October 11, 2023 19:11

juj reviewed Oct 16, 2023

View reviewed changes

src/library_atomic.js Outdated Show resolved Hide resolved

juj reviewed Oct 16, 2023

View reviewed changes

sbc100 mentioned this pull request Oct 17, 2023

Separate out Atomics.waitAsync polyfill. NFC #20474

Merged

sbc100 force-pushed the move_wait_async branch from 1fe2c44 to 736067d Compare October 19, 2023 16:41

sbc100 force-pushed the move_wait_async branch from 736067d to 02eb840 Compare October 19, 2023 16:44

sbc100 requested review from juj and kripken October 19, 2023 16:48

kripken reviewed Oct 19, 2023

View reviewed changes

kripken approved these changes Oct 19, 2023

View reviewed changes

sbc100 force-pushed the move_wait_async branch from 02eb840 to 9649f90 Compare October 23, 2023 17:21

Move emscripten_atomic_wait_async to atomic.h and add core testing

06f2e36

This API was orininally added for wasm workers but is not wasm worker specific. Followup to emscripten-core#20381.

sbc100 force-pushed the move_wait_async branch from 9649f90 to 06f2e36 Compare October 23, 2023 23:46

sbc100 enabled auto-merge (squash) October 23, 2023 23:46

sbc100 merged commit a0a3f24 into emscripten-core:main Oct 24, 2023
2 checks passed

sbc100 deleted the move_wait_async branch October 24, 2023 00:54

wenhanchong mentioned this pull request Nov 28, 2024

PThreads unable to initialize on Firefox after 3.1.48 #23032

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `emscripten_atomic_wait_async` to atomic.h and add core testing #20404

Move `emscripten_atomic_wait_async` to atomic.h and add core testing #20404

sbc100 commented Oct 5, 2023 •

edited

Loading

juj Oct 16, 2023

juj Oct 16, 2023 •

edited

Loading

sbc100 Oct 17, 2023

sbc100 Oct 17, 2023

sbc100 Oct 17, 2023

sbc100 Oct 19, 2023

sbc100 commented Oct 19, 2023

kripken left a comment

sbc100 commented Oct 19, 2023

sbc100 commented Oct 19, 2023

kripken commented Oct 19, 2023

sbc100 commented Oct 19, 2023

kripken commented Oct 19, 2023 •

edited

Loading

Move emscripten_atomic_wait_async to atomic.h and add core testing #20404

Move emscripten_atomic_wait_async to atomic.h and add core testing #20404

Conversation

sbc100 commented Oct 5, 2023 • edited Loading

juj Oct 16, 2023

Choose a reason for hiding this comment

juj Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

sbc100 Oct 17, 2023

Choose a reason for hiding this comment

sbc100 Oct 17, 2023

Choose a reason for hiding this comment

sbc100 Oct 17, 2023

Choose a reason for hiding this comment

sbc100 Oct 19, 2023

Choose a reason for hiding this comment

sbc100 commented Oct 19, 2023

kripken left a comment

Choose a reason for hiding this comment

sbc100 commented Oct 19, 2023

sbc100 commented Oct 19, 2023

kripken commented Oct 19, 2023

sbc100 commented Oct 19, 2023

kripken commented Oct 19, 2023 • edited Loading

Move `emscripten_atomic_wait_async` to atomic.h and add core testing #20404

Move `emscripten_atomic_wait_async` to atomic.h and add core testing #20404

sbc100 commented Oct 5, 2023 •

edited

Loading

juj Oct 16, 2023 •

edited

Loading

kripken commented Oct 19, 2023 •

edited

Loading