[WIP] Experiment with atomic builtins on Node #15740
Conversation
| } else { | ||
| // Can wait in one go. | ||
| #ifdef __EMSCRIPTEN_ATOMIC_BUILTINS__ | ||
| __builtin_wasm_memory_atomic_wait32((int*)addr, val, -1); | ||
| #else // !defined(__EMSCRIPTEN_ATOMIC_BUILTINS__) | ||
| emscripten_futex_wait((void*)addr, val, INFINITY); |
There was a problem hiding this comment.
But doesn't emscripten_futex_wait already used __builtin_wasm_memory_atomic_wait32 in places where this is legal (i.e. off the main web thread?)
I think the decision on whether we can use the atomic wait needs to be dynamic doesn't it ? We don't actually want to produce two different binaries do we?
There was a problem hiding this comment.
emscripten_futex_wait is implemented in JS and calls directly Atomics.wait for non-web environments. So, this PR is NFC in that respect.
This does indeed produce a different binary if you only target Node, so that would be a breaking change. Perhaps it would be better to implement emscripten_futex_wait dynamically in native code. But as far as I know, that is currently not possible since a JS equivalent of ENVIRONMENT_IS_WEB is not available (PR #15659 is also relevant here).
There was a problem hiding this comment.
I think we can/should implement something like emscripten_thread_can_block in native code.
There was a problem hiding this comment.
I decided to have a go addingemscripten_thread_can_block along with native version of emscripten_futex_wait: #15742
There was a problem hiding this comment.
Great! Do you plan to move emscripten_futex_wake to native code as well (which could use the __builtin_wasm_memory_atomic_notify builtin)?
There was a problem hiding this comment.
Yes I hope we can move all the wait/wait to native code but I'm doing it one step at a time,
|
Closing in favor of #15742, which is a better approach. |
This PR experiments with the use of atomic builtins on non-web environments (for e.g. Node.js). I did a quick test on the POSIX test suite, to check if this is worthwhile:
Before:
Ran 383 tests in 213.139s OK (skipped=23, expected failures=53)After:
Ran 383 tests in 212.284s OK (skipped=23, expected failures=53)But benchmarking this on the test suite is probably not a good measurement.
This PR is currently easiest to review per commit, as it depends upon PR #15739.
Context: #15727.