-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Allow pthreads on Node.js without a pthread pool #18305
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
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe someone else can review this more easily, but for me at least I don't recall all the async startup logic, so I could benefit from an overview of the old way and the new way proposed here.
Let me try to do a TL;DR summary by using a Before:
After:
What this reordering means on practice in Node.js is that before this change the main thread couldn't block synchronously right after After this change, the main thread doesn't wait for that "loaded" message anymore and instead sends the "run" command immediately in the same event loop tick, putting the responsibility onto the Worker to only execute the code after it's fully loaded. This means main thread can safely block right after the Hope this helps. |
b92c75f
to
d58e058
Compare
@RReverser In the "after" list, steps 2 and 3 are both "Main thread sends a message" - could it not send a single message for both? |
They're not always sent together. In this particular example - pthread_create when a threadpool is not ready - yes, it will send 2 messages. However, in other cases, e.g. when preparing a threadpool, there will be only "load" message, and when doing pthread_create with a ready pool, there will be only "run". It's possible to add 3rd kind of message for combined "loadAndRun", but it would complicate code further and won't add any perf benefits because those messages, when they happen to be sent both at once, are already sent in the same event loop tick. |
aeadff5
to
1ad37f5
Compare
Rebased & added a changelog line. Are there any blocking changes here left? |
Node.js `worker_threads` are different than browser `Worker`s in that they start spawning synchronously without waiting for an event loop tick. If we can also avoid waiting for the "loaded" event from the worker before sending pthread messages to it, then we can spawn new pthreads "synchronously" (prepare everything within the same event loop tick), and block the current thread, making the behaviour on Node.js a lot closer to native and avoiding the need for a pthread pool even without Asyncify or extra helper workers. That's what I did in this implementation. Instead of waiting for the worker to tell us it's loaded and ready, I'm sending all commands immediately to the worker. The worker accepts the first "load" message, starts initializing the runtime, and meanwhile queues up any further messages such as "run". Once the runtime is ready, it processes the queue. I could limit those changes only to Node.js, but it's easier to do both together, allows to avoid a custom `worker.runPthread` callback, and should be in theory even a bit faster in browsers too (by avoiding the "loaded" roundtrip).
1ad37f5
to
bee58ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So IIRC that pool pre-allocation still happens under node, even though its not really necessary?
I do like being able to test stuff under node, so I guess it is still useful, but should we consider having the re-allocated pool be something that only ever happens in the browser?
If user specifies
I think it makes sense to still respect user choice here; if they explicitly specify PTHREAD_POOL_SIZE, they might want the faster startup for pthreads. Once we add some way to avoid pthread pool in browsers too, we can document that PTHREAD_POOL_SIZE is no longer necessary if your only goal is to avoid deadlocks, and gradually migrate users away from it. I don't want to completely ignore the option just for one target as part of this PR though. |
I see, thanks, that's what I was missing. |
Glad it clears it up, sorry for the confusion. I used that particular scenario to show differences in workflows because it's the scenario that would deadlock before this change but won't now; other mentioned scenarios are less interesting because they worked anyway. |
The corresponding TODO-item is no longer relevant after PR emscripten-core#18305 has landed. Resolves: emscripten-core#9763.
Node.js
worker_threads
are different than browserWorker
s in that they start spawning synchronously without waiting for an event loop tick.If we can also avoid waiting for the
"loaded"
event from the worker before sending pthread tasks to it, then we can spawn new pthreads "synchronously" (prepare everything within the same event loop tick), and block the current thread, making the behaviour on Node.js a lot closer to native and avoid the need for a pthread pool even without Asyncify or extra helper workers.That's what I did in this implementation. Instead of waiting for the worker to tell us it's loaded and ready, I'm sending all commands immediately to the worker. The worker accepts the first "load" message, starts initializing the runtime, and meanwhile queues up any further messages such as
"run"
. Once the runtime is ready, it processes the queue.I could limit those changes only to Node.js, but it's easier to do both together, as it allows to avoid a custom
worker.runPthread
callback, and should be in theory even a bit faster in browsers too (by avoiding the "loaded" roundtrip before running a pthread).