Skip to content

Conversation

@sam-github
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 23, 2017
@sam-github
Copy link
Contributor Author

Did I get all the threadpool-using APIs, @bnoordhuis ?

doc/api/cli.md Outdated
Copy link
Contributor

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
Copy link
Contributor

@mscdex mscdex Aug 23, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

@jasnell jasnell Aug 23, 2017

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?

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Aug 23, 2017

If V8 is now using only the libuv threadpool, then that probably includes other things as well, at least the optimizer and GC...

@matthewloring
Copy link

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.

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

@matthewloring ... Thank you for the clarification!! :-) It may still be worth mentioning that somehow!

@sam-github
Copy link
Contributor Author

@jasnell @mscdex PTAL

@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
Copy link
Contributor

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').

@mscdex
Copy link
Contributor

mscdex commented Aug 24, 2017

Just one more minor nit, but otherwise LGTM.

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

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!

@sam-github
Copy link
Contributor Author

Thanks for the reviews.

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github sam-github merged commit a1d34b3 into nodejs:master Aug 25, 2017
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github sam-github deleted the doc-threadpool branch September 7, 2017 20:49
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants