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

Feature Request: Support Worker Threads using Sharp #1558

Closed
metabench opened this issue Jan 27, 2019 · 11 comments
Closed

Feature Request: Support Worker Threads using Sharp #1558

metabench opened this issue Jan 27, 2019 · 11 comments

Comments

@metabench
Copy link

@lovell While I have read how Sharp itself would not benefit from worker threads in #1297, I am using Sharp alongside some CPU-intensive JS programming that certainly would benefit from being in a worker thread.

nodejs/node#21481 explains the issue, and at the bottom is a reference to how it was solved in node-lzo

@lovell
Copy link
Owner

lovell commented Jan 27, 2019

Hello, sharp uses nan's NAN_MODULE_INIT and this feels like something that might be better added in that module so many native modules can benefit, especially as the NODE_MODULE_INIT macro is only available in Node 10.7.0+.

Having said that I'd be happy to accept a PR that does this as long as it remains fairly simple and has tests.

The lzo example does not appear to use libuv worker threads where sharp does so may not be a complete enough example.

The "context-aware addon carries with it the responsibility of carefully managing global static data" warning of https://nodejs.org/api/addons.html#addons_context_aware_addons is relevant here too.

@metabench
Copy link
Author

Is it worth still using nan or shifting to N-API?

@lovell
Copy link
Owner

lovell commented Feb 20, 2019

Please see #1282 for N-API.

@nstepien
Copy link
Contributor

@lovell
Copy link
Owner

lovell commented May 13, 2019

There's a discussion about adding this logic to nan at nodejs/nan#844

@vinerz
Copy link

vinerz commented May 21, 2019

+1. I am still suffering from some non-heap leakage at heroku for over a year now.
Would workers be a benefit for me as they eventually die?
I'm currently spawning a new child_process, which fixes the leak, but the overhead is unbelievable.
The request time went from 140ms to ~1500ms after this workaround.

EDIT: jemalloc in fact did the job and fixed the memory issues. Hooray!
For anyone wondering how to do that in Heroku, use the jemalloc buildpack.

@reutopiaer
Copy link

I can user sharp in Worker Threads (sharp:v0.22.1,node:v10.15.3).
But when I require sharp in mainThread and workerThread both, I still get error: "Module did not self-register."

@lovell lovell added this to the v0.23.0 milestone Jun 26, 2019
@lovell
Copy link
Owner

lovell commented Jun 26, 2019

Commit 946d3c8 makes the switch to use the new NAN_MODULE_WORKER_ENABLED macro. This will be in v0.23.0.

I've flagged this as experimental in the changelog as although both sharp and libvips are very much thread-aware, this probably requires testing at scale before use in production.

@lovell
Copy link
Owner

lovell commented Jul 29, 2019

sharp v0.23.0 is now available.

@lovell lovell closed this as completed Jul 29, 2019
@MarcusCemes
Copy link

MarcusCemes commented May 20, 2020

Hi,
Would anybody know by any chance if worker threads have their own libuv threadpool? In my tests, worker threads share the exact same sharp job queue, which is limited to 4 concurrent images (without setting UV_THREADPOOL_SIZE) and can even be queried from the main thread (sharp.counters()). Is there any way to give each thread its own set of processing threads? I have a code sample if someone is interested.

My current solution is to spawn more Node.js process, but threads would be lighter. I don't want to bug my user to set UV_THREADPOOL_SIZE either for a CLI tool.

@dnlup
Copy link

dnlup commented May 20, 2020

Hi,
Would anybody know by any chance if worker threads have their own libuv threadpool? In my tests, worker threads share the exact same sharp job queue, which is limited to 4 concurrent images (without setting UV_THREADPOOL_SIZE) and can even be queried from the main thread (sharp.counters()). Is there any way to give each thread its own set of processing threads? I have a code sample if someone is interested.

My current solution is to spawn more Node.js process, but threads would be lighter. I don't want to bug my user to set UV_THREADPOOL_SIZE either for a CLI tool.

@MarcusCemes worker threads all share the same thread pool, you can't much about it. You could try to use N workers, and in each worker set sharp concurrency to 1, but I don't think the performance would be better.

eventualbuddha added a commit to votingworks/hmpb-interpreter that referenced this issue Sep 29, 2020
canvas does not support being loaded in worker threads due to it being a native dependency that was not written using a thread-compatible API. sharp, on the other hand, is thread-aware: lovell/sharp#1558.
eventualbuddha added a commit to votingworks/vxsuite that referenced this issue Oct 28, 2020
canvas does not support being loaded in worker threads due to it being a native dependency that was not written using a thread-compatible API. sharp, on the other hand, is thread-aware: lovell/sharp#1558.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants