Skip to content
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

Node Worker pthreads pool reuse #9763

Closed
kripken opened this issue Nov 1, 2019 · 8 comments · Fixed by #19178
Closed

Node Worker pthreads pool reuse #9763

kripken opened this issue Nov 1, 2019 · 8 comments · Fixed by #19178

Comments

@kripken
Copy link
Member

kripken commented Nov 1, 2019

#9745 supports Node Workers and the pthread pool works properly, but the pool elements are not reused. For example,

https://github.com/emscripten-core/emscripten/blob/incoming/tests/pthread/test_pthread_create.cpp

creates many many threads and destroys them (so at every point in time only a small amount exist). This hangs on node as after exhausting the pool, we have no more workers to use.

I think the issue is that pthread_exit in workers actually exits the entire Worker in node. We call the node process API to do that, as it's better than throwing an exception - which on the Web halts the worker but leaves the application alone, but in Node brings down the whole thing. So I guess we need to find a way to do what we do on the Web, in Node.

@kripken kripken changed the title Node Worker pool reuse Node Worker pthreads pool reuse Nov 1, 2019
@dschuff
Copy link
Member

dschuff commented Nov 2, 2019

Or just do what native thread libraries do, i.e. actually create a new thread. I'll bet it's a lot cheaper in Node than it is on the web.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 2, 2019

I agree. Lets just not do the pool thing on node. Creating a pool of threads in userspace is possible if people what to do that (which is also common in the non-web world).

@kripken
Copy link
Member Author

kripken commented Nov 4, 2019

The problem is that creating the pool is a workaround for the issue of expecting a thread to immediately be usable. That is, creating a thread is an async operation, so the main thread needs to return to the event loop and wait.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 4, 2019

But is that true with node as well? Don't we have synchronous primitives under node?

@kripken
Copy link
Member Author

kripken commented Nov 4, 2019

It seems to still be true in Node.js, yes, creating a Worker doesn't make it immediately available, the event loop must be returned to. I don't see an API to synchronously create and run a Worker. But maybe I missed something, cc @addaleax

@addaleax
Copy link

addaleax commented Nov 5, 2019

I don't see an API to synchronously create and run a Worker. But maybe I missed something, cc @addaleax

There’s nothing built-in, but if you want something like this, you could pass an Int32Array over a SharedArrayBuffer in the workerData option to the Worker constructor, and use Atomics.wait() on the main thread and Atomics.notify() once you want to wake up the main thread from that wait call. (I’d be curious why you need the thread to be immediately usable in that case, though.)

I think the issue is that pthread_exit in workers actually exits the entire Worker in node. We call the node process API to do that, as it's better than throwing an exception - which on the Web halts the worker but leaves the application alone, but in Node brings down the whole thing. So I guess we need to find a way to do what we do on the Web, in Node.

You can still throw an exception if that’s what you want. If you want it to not bring down the Worker entirely, you can attach a process.on('uncaughtException', ...) listener inside of the Worker thread.

@stale
Copy link

stale bot commented Nov 5, 2020

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 17, 2022
kleisauke added a commit to kleisauke/emscripten that referenced this issue Apr 14, 2023
The corresponding TODO-item is no longer relevant after PR emscripten-core#18305
has landed.

Resolves: emscripten-core#9763.
sbc100 pushed a commit that referenced this issue Apr 14, 2023
The corresponding TODO-item is no longer relevant after PR #18305
has landed.

Resolves: #9763.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants