-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: threadpool size, and APIs using the pool #14995
Conversation
Did I get all the threadpool-using APIs, @bnoordhuis ? |
doc/api/cli.md
Outdated
|
||
- `fs.*()` | ||
- `dns.lookup()` | ||
- `crypto.randomBytes()` |
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.
An asterisk or something should be added here to note this is only when a callback is supplied, which activates async mode.
doc/api/cli.md
Outdated
- `fs.*()` | ||
- `dns.lookup()` | ||
- `crypto.randomBytes()` | ||
- `crypto.randomFill()` |
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.
We're also missing the asynchronous zlib
functions. For example:
zlib.deflate()
zlib.deflateRaw()
zlib.inflate()
zlib.inflateRaw()
zlib.gzip()
zlib.gunzip()
zlib.unzip()
Also, we're missing crypto.pbkdf2()
.
There could be others I'm missing, so this might warrant a more thorough investigation to be extra sure this is all of them.
doc/api/cli.md
Outdated
do not exist, libuv's threadpool is used to create asynchronous node APIs based | ||
on synchronous system APIs. Node.js APIs that use the threadpool are: | ||
|
||
- `fs.*()` |
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.
Minor nit: this is really fs.*()
with the exception of fs.*Sync()
.
doc/api/cli.md
Outdated
- `crypto.randomFill()` | ||
|
||
Because libuv's threadpool has a fixed size, it means that if for whatever | ||
reason any of these APIs takes a long time, other (seemingly unrelated) APIS |
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.
s/APIS/APIs/
doc/api/cli.md
Outdated
|
||
Because libuv's threadpool has a fixed size, it means that if for whatever | ||
reason any of these APIs takes a long time, other (seemingly unrelated) APIS | ||
that run on libuv's threadpool will experience degraded performance. In order to |
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.
minor nit: s/on/in/
doc/api/dns.md
Outdated
Note that various networking APIs will call `dns.lookup()` internally to resolve | ||
host names, but that they will not do this if the host names were resolved into | ||
addresses using `dns.resolve()`. This may allow some applications to avoid using | ||
the threadpool altogether for address lookups. |
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.
It might be beneficial to add a short note that some functions accept a custom lookup
function (e.g. net.Socket
and now dgram
both support this feature), so that you can more easily share a function that calls dns.resolve()
instead.
doc/api/fs.md
Outdated
@@ -100,6 +100,13 @@ example `fs.readdirSync('c:\\')` can potentially return a different result than | |||
`fs.readdirSync('c:')`. For more information, see | |||
[this MSDN page][MSDN-Rel-Path]. | |||
|
|||
## Threadpool Usage | |||
|
|||
Note that all file system APIs except `fs.FSWatcher()` use libuv's threadpool, |
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.
As @mscdex notes, the *Sync
variants do not, correct?
With the very recent addition of the Node implementation of v8::Platform, I believe the V8 microtask queue for Promises also now uses the libuv threadpool if I'm not mistaken. /cc @matthewloring |
If V8 is now using only the libuv threadpool, then that probably includes other things as well, at least the optimizer and GC... |
The Node implementation of v8::Platform originally used the libuv threadpool but the final implementation manages its own independent pool of libuv threads. The size of that pool is configured here: https://github.com/nodejs/node/blob/master/src/node.cc#L261. |
@matthewloring ... Thank you for the clarification!! :-) It may still be worth mentioning that somehow! |
@mscdex I searched for usage of uv_queue_work, I think this has all the thread pool APIs, thanks for the reminder about the kdf and zlib @jasnell I don't know anything about the v8 threadpool, and it doesn't have any interaction with libuvs, I'll leave documenting it to someone else (if it needs docs, I'm not even sure if its existence is user visible). |
doc/api/dns.md
Outdated
host names. If that is an issue, consider resolving the hostname to and address | ||
using `dns.resolve()` and using the address instead of a host name. Also, some | ||
networking APIs (such as [`socket.connect()`][] and [`dgram.createSocket()`][]) | ||
allow the default resolver, `dns.lookup()` to be replaced. |
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.
Minor nit: i think there should either be another comma after `dns.lookup()` or it should be placed in parentheses instead ('allow the default resolver (`dns.lookup()`) to be replaced').
Just one more minor nit, but otherwise LGTM. |
That's fair @sam-github I'd like to see if we can pull together a more comprehensive guide on this stuff as a separate doc. This pr LGTM! |
Thanks for the reviews. |
6b58176
to
0e4f4b8
Compare
Not knowing which APIs use libuv's threadpool can lead to surprising performance problems. Document the APIs, and also document UV_THREADPOOL_SIZE, which can be used to fix problems. PR-URL: nodejs#14995 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com>
0e4f4b8
to
a1d34b3
Compare
Not knowing which APIs use libuv's threadpool can lead to surprising performance problems. Document the APIs, and also document UV_THREADPOOL_SIZE, which can be used to fix problems. PR-URL: nodejs/node#14995 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Not knowing which APIs use libuv's threadpool can lead to surprising performance problems. Document the APIs, and also document UV_THREADPOOL_SIZE, which can be used to fix problems. PR-URL: nodejs/node#14995 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Not knowing which APIs use libuv's threadpool can lead to surprising performance problems. Document the APIs, and also document UV_THREADPOOL_SIZE, which can be used to fix problems. PR-URL: #14995 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Not knowing which APIs use libuv's threadpool can lead to surprising performance problems. Document the APIs, and also document UV_THREADPOOL_SIZE, which can be used to fix problems. PR-URL: #14995 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Not knowing which APIs use libuv's threadpool can lead to surprising
performance problems. Document the APIs, and also document
UV_THREADPOOL_SIZE, which can be used to fix problems.
Checklist
Affected core subsystem(s)
doc