Repository navigation
Sync threads in parallel when dlopen runs on the main runtime thread - #28176
rustyconover wants to merge 2 commits into
Conversation
_emscripten_dlsync_threads waited for each thread in turn, so the threads loaded a new side module one after another and a thread that was slow to yield held up all the others. Ask every thread to sync first, then wait for all of them, as dlsyncThreadsAsync already does for dlopen on a worker. The completion callbacks add traffic on _dlopen_proxying_queue. do_proxy can allocate with that queue's mutex held, and a contended malloc can yield into emscripten_proxy_execute_queue, which takes the mutex again. Guard every queue against that, not only the system queue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sbc100
left a comment
There was a problem hiding this comment.
The idea of doing this in parallel seems perfectly fine.
The changes to proxying.c might need some deeper scrutiny though since that subsystem is very fragile/important/suble. @tlively will likely need to be involved the review of that.
There was a problem hiding this comment.
Code Review
This pull request refactors thread synchronization during dlopen to occur in parallel rather than sequentially, preventing potential deadlocks when some threads are busy. It replaces _emscripten_proxy_dlsync with _emscripten_proxy_dlsync_post and _emscripten_proxy_dlsync_wait, and addresses a potential deadlock in proxying.c during memory allocation with a queue's mutex held. A new test is added to verify parallel synchronization. Feedback on the new test file recommends explicitly including <emscripten.h> for emscripten_get_now() and removing the unused done variable.
| #include <assert.h> | ||
| #include <dlfcn.h> | ||
| #include <emscripten/em_asm.h> | ||
| #include <emscripten/threading.h> | ||
| #include <pthread.h> | ||
| #include <stdatomic.h> | ||
| #include <stdio.h> |
There was a problem hiding this comment.
The function emscripten_get_now() is used in exiting_thread but <emscripten.h> is not explicitly included. Although it might be transitively included by other headers, it is best practice to explicitly include <emscripten.h> to ensure the declaration is always available and to prevent compilation issues if transitive headers change.
#include <assert.h>
#include <dlfcn.h>
#include <emscripten.h>
#include <emscripten/em_asm.h>
#include <emscripten/threading.h>
#include <pthread.h>
#include <stdatomic.h>
#include <stdio.h>| _Atomic int loading; | ||
| _Atomic int loaded_on_thread; | ||
| _Atomic int done; |
| // Adding a task can allocate with the queue's mutex held, and a contended | ||
| // malloc can get here from emscripten_yield. Taking the mutex again would | ||
| // deadlock this thread, so leave the queue to be executed later. |
There was a problem hiding this comment.
Unfortunately delaying the queue execution to later can result in missed wakeups and new deadlocks, so we'll need a different solution here. I will work on this and upload a PR today.
Depends on #28047. Without that fix, the new test hangs in about 5% of stress runs (73 of 1,500), so it can flake in CI until #28047 lands.
Problem
When
dlopenruns on the main runtime thread,_emscripten_dlsync_threadswaits for each thread in turn. The threads load the new side module one after another, sodlopengets slower with every running thread, and a thread that is slow to yield holds up all the others.Change
dlsyncThreadsAsyncalready works this way fordlopenon a worker, but itsawaitcan't be used on this path, which has to block.proxying.c:do_proxycan allocate with the queue's mutex held, and a contended malloc can yield intoemscripten_proxy_execute_queue, which takes the same mutex again. The code already guards against this for the system queue; this covers every queue. The existing async path has the same exposure.Results
Time to
LOADsix extensions in a DuckDB-wasm based app (Haybarn), threaded build, median of 4–10 runs. These used a backport to 6.0.13 that includes #28047. Startup time didn't change.Testing
These ran on aarch64 Linux with Node 24.19.0, and all but the mode run below were on
mainat7c2d477, with #28047 applied:test_pthread_dlopen_parallel_synchas two threads that don't yield waiting on a third that must sync first, so it hangs on the old loop. An earlier version of the test was checked against the old loop and did hang there.test_pthread_dlopenand Release the task queue lock while running cancel handlers #28047'stest_pthread_dlopen_exitpass incore0,core2,corez,asan,lsan,wasm64andcore_2gb. This exact branch, without Release the task queue lock while running cancel handlers #28047 and on currentmain, passes the first two incore0,core2,corez,wasm64andcore_2gb.main(test_pthread_wait_suspending*andtest_pthread_join_and_asyncifyincore2andasan).get_or_add_tasks_for_thread, 5 of 100 runs hung without the guard and 0 of 100 with it. That injection can't be committed, so there's no regression test for the guard. If you'd rather avoid allocating under the lock, or see this as its own PR, I can do either.Not run locally: the browser test suite.
Known limitation
If memory runs out and the completion can't be sent back to the main thread,
dlsync_pendingis never decremented and the wait hangs. The old synchronous path didn't allocate at that point. The async path doesn't appear to resolve its promise in that case either.Conflicts
This overlaps #28007 and #28056 on the same lines. Whichever lands second needs a mechanical rebase.
🤖 Generated with Claude Code