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.

Sign up for GitHub

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

Jump to bottom

doc,zlib: improve note on threadpool usage #20380

Closed
wants to merge 1 commit into from
Closed

doc,zlib: improve note on threadpool usage #20380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions doc/api/zlib.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ zlib.unzip(buffer, (err, buffer) => {
## Threadpool Usage

Note that all zlib APIs except those that are explicitly synchronous use libuv's
threadpool, which can have surprising and negative performance implications for
some applications, see the [`UV_THREADPOOL_SIZE`][] documentation for more
information.
threadpool. This can lead to surprising effects in some applications, such as
subpar performance (which can be mitigated by adjusting the [pool size][])
and/or unrecoverable and catastrophic memory fragmentation.
Copy link
Member

Choose a reason for hiding this comment

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

IMO this warning is a bit lax. I think it would be best to give a more detailed explanation of what is wrong and how to mitigate it (CPU bound tasks need their time one way or the other, so a solution could be to use a separate Node.js instance as a worker that is connected to the main application with a queue and the queue sends new tasks as soon as the worker is done with one entry).

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR Can you expand on that? I can’t really make out a difference between what you’re describing and how the libuv event loop works right now…

Copy link
Member

Choose a reason for hiding this comment

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

So, what how I understood the issue so far is that the actual call to the async functions cause the problem. The reason is that the task itself is CPU bound and if we trigger lots of async calls, we end up with catastrophic memory fragmentation. The libuv event loop can not prevent that each call will at least allocate some memory.

I just suggest to document that it is best to only have a single worker for n Node.js instances that will handle all the async tasks. The single worker could actually process m tasks in parallel, while m stands for the number of CPU cores. That should mitigate the issue, if I am not mistaken. Besides that we might want to re-evaluate the recommendation to always use async calls in case the actual work will be CPU bound. Using sync calls will definitely not cause this problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BridgeAR In ws we are doing something similar. We use a queue to limit the maximum number of concurrent calls to zlib: https://github.com/websockets/ws/blob/690b3f277c6f5c3aef8cd84792929450f516b3ae/lib/permessage-deflate.js#L67-L73.

It helps but according to this comment even setting concurrency to 1 does not fully fixes the issue. Your suggestion can help with applications but it's a bit impractical for libraries.

Also the point of this PR is to only make people aware of the "issue". A detailed explanation of why and where it happens and how to mitigate it's out of the scope of this PR as that would require multiple pages of docs.

Copy link

Choose a reason for hiding this comment

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

This is really a terrible warning. It is just scary works with no explanation or advice. You may as well have said "Warning: if you use this code you may die. Good luck!"


## Compressing HTTP requests and responses

Expand Down Expand Up @@ -777,9 +777,9 @@ Decompress a chunk of data with [`Unzip`][].
[`Inflate`]: #zlib_class_zlib_inflate
[`InflateRaw`]: #zlib_class_zlib_inflateraw
[`TypedArray`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
[`UV_THREADPOOL_SIZE`]: cli.html#cli_uv_threadpool_size_size
[`Unzip`]: #zlib_class_zlib_unzip
[`options`]: #zlib_class_options
[`zlib.bytesWritten`]: #zlib_zlib_byteswritten
[Memory Usage Tuning]: #zlib_memory_usage_tuning
[pool size]: cli.html#cli_uv_threadpool_size_size
[zlib documentation]: https://zlib.net/manual.html#Constants