Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 13, 2021

This is great example of why moving stuff into native code can save on overall size:

jssize: 49696 -> 48861 = -775
wasm size: 16846 -> 16928 = +82

@sbc100 sbc100 force-pushed the emscripten_futex_wake branch 8 times, most recently from 58ebb04 to d181bf5 Compare December 14, 2021 16:36
@kleisauke
Copy link
Collaborator

Some tests (e.g. browser.test_embind_with_pthreads) are failing with this assertion.

Aborted(Assertion failed: !_emscripten_thread_supports_atomics_wait(), at: /home/kleisauke/emscripten/system/lib/pthread/emscripten_futex_wake.c,37,emscripten_futex_wake)

Which is curious, I tried to solve that with this simplification:

15766.patch
diff --git a/system/lib/pthread/emscripten_futex_wake.c b/system/lib/pthread/emscripten_futex_wake.c
index ac3a42eba..6e87bd5ed 100644
--- a/system/lib/pthread/emscripten_futex_wake.c
+++ b/system/lib/pthread/emscripten_futex_wake.c
@@ -8,12 +8,14 @@
 #include <assert.h>
 #include <errno.h>
 #include <limits.h>
-#include <stdatomic.h>
+#include <atomic.h>
 #include <emscripten/threading.h>
 
 int _emscripten_thread_supports_atomics_wait(void);
-extern void* _Atomic _emscripten_main_thread_futex;
 
+// Stores the memory address that the main thread is waiting on, if any. If
+// the main thread is waiting, we wake it up before waking up any workers.
+EMSCRIPTEN_KEEPALIVE void *volatile _emscripten_main_thread_futex;
 
 // Returns the number of threads (>= 0) woken up, or the value -EINVAL on error.
 // Pass count == INT_MAX to wake up all threads.
@@ -29,19 +31,16 @@ int emscripten_futex_wake(volatile void *addr, int count) {
   // Note that this is not a fair procedure, since we always wake main thread first before any workers, so
   // this scheme does not adhere to real queue-based waiting.
   int main_thread_woken = 0;
-  if (_emscripten_main_thread_futex == addr) {
+  if (a_cas_p(&_emscripten_main_thread_futex, (void *)addr, 0) == addr) {
     // We only use __emscripten_main_thread_futex on the main browser thread,
     // where we cannot block while we wait. Therefore we should only see it set
     // from other threads, and not on the main thread itself. In other words,
     // the main thread must never try to wake itself up!
     assert(!_emscripten_thread_supports_atomics_wait());
-    void* expected = (void*)addr;
-    if (atomic_compare_exchange_strong(&_emscripten_main_thread_futex, &expected, 0)) {
-      --count;
-      main_thread_woken = 1;
-      if (count <= 0) {
-        return 1;
-      }
+    --count;
+    main_thread_woken = 1;
+    if (count <= 0) {
+      return 1;
     }
   }
 
diff --git a/system/lib/pthread/library_pthread.c b/system/lib/pthread/library_pthread.c
index 3d5051f04..612e697d7 100644
--- a/system/lib/pthread/library_pthread.c
+++ b/system/lib/pthread/library_pthread.c
@@ -831,10 +831,6 @@ int emscripten_dispatch_to_thread_async_(pthread_t target_thread,
   return ret;
 }
 
-// Stores the memory address that the main thread is waiting on, if any. If
-// the main thread is waiting, we wake it up before waking up any workers.
-EMSCRIPTEN_KEEPALIVE void* _emscripten_main_thread_futex;
-
 void __emscripten_init_main_thread_js(void* tb);
 
 static void *dummy_tsd[1] = { 0 };

To no avail (same assertion failure).

@sbc100 sbc100 force-pushed the emscripten_futex_wake branch 2 times, most recently from 2aa6eb4 to 9a8451d Compare December 14, 2021 22:52
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2021

Ah, it looks like I had the condition in the assert the wrong way around..

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2021

I like your simplification though! I will apply that..

@sbc100 sbc100 force-pushed the emscripten_futex_wake branch from 9a8451d to 8000992 Compare December 14, 2021 22:56
@sbc100 sbc100 force-pushed the emscripten_futex_wake branch from 8000992 to fefc22b Compare December 14, 2021 23:13
@sbc100 sbc100 requested a review from juj December 14, 2021 23:14
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 15, 2021

I ran the whole of posixtest and posixtest_browser so I'm pretty confident the translation is good.

@sbc100 sbc100 enabled auto-merge (squash) December 15, 2021 01:16
@sbc100 sbc100 merged commit 3dbfca3 into main Dec 15, 2021
@sbc100 sbc100 deleted the emscripten_futex_wake branch December 15, 2021 01:29
// from other threads (that should always support waiting), and not on the
// main thread itself. In other words, the main thread must never try to
// wake itself up!
assert(_emscripten_thread_supports_atomics_wait());
Copy link
Member

Choose a reason for hiding this comment

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

At some point we might want to audit our code to try to make it work correctly from audio worklets, which also can't block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah.. this mechanism assumes only one (busy-looping) main thread I think, so it would need to be revisited.

Copy link
Contributor

@Jonathhhan Jonathhhan Oct 6, 2022

Choose a reason for hiding this comment

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

I guess I came across the audioworklet issue:

SimpleSequencer.js:1 Uncaught RuntimeError: Aborted(Assertion failed: _emscripten_thread_supports_atomics_wait(), at: 
/home/jonathan/emsdk/upstream/emscripten/system/lib/pthread/emscripten_futex_wake.c,40,emscripten_futex_wake)
    at abort (SimpleSequencer.js:1:23555)
    at ___assert_fail (SimpleSequencer.js:1:92185)
    at emscripten_futex_wake (SimpleSequencer.wasm:0x5fcc66)
    at __wake (SimpleSequencer.wasm:0x5fc80b)
    at __pthread_mutex_unlock (SimpleSequencer.wasm:0x6041a7)
    at sys_unlock (SimpleSequencer.wasm:0x4b4743)
    at ofxPd::audioOut(float*, int, int) (SimpleSequencer.wasm:0x43abb3)
    at ofxOfelia::audioOut(ofSoundBuffer&) (SimpleSequencer.wasm:0x691b3)
    at std::__2::__function::__func<std::__2::__bind<void (ofBaseSoundInput::*)(ofSoundBuffer&), ofBaseSoundInput*&, std::__2::placeholders::__ph<1> const&>, std::__2::allocator<std::__2::__bind<void (ofBaseSoundInput::*)(ofSoundBuffer&), ofBaseSoundInput*&, std::__2::placeholders::__ph<1> const&>>, void (ofSoundBuffer&)>::operator()(ofSoundBuffer&) (SimpleSequencer.wasm:0x63b62)
    at ofxEmscriptenSoundStream::audio_cb(int, int, int, void*) (SimpleSequencer.wasm:0x637e0)

It would be absolutely great, if this gets fixed. Because the app I am using with Emscripten seems to rely on that. And I am a bit lost with fixing it by myself.

mmarczell-graphisoft pushed a commit to GRAPHISOFT/emscripten that referenced this pull request Jan 5, 2022
This is great example of why moving stuff into native code can save on overall size:
 
jssize: 49696 -> 48861  = -775
wasm size: 16846 -> 16928 = +82
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.

6 participants