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

src: use thread defaults provided by v8::platform #4243

Closed
wants to merge 1 commit into from

Conversation

tomgco
Copy link
Contributor

@tomgco tomgco commented Dec 11, 2015

By passing const int thread_pool_size = 0 to CreateDefaultPlatform
we allow v8 to choose sensible defaults based on the currently online
processors.

Benchmarking on my local machine saw no performance regressions; however it is dual core and hyper-threaded and would of used the same thread count as previously - I assume that this will benefit machines with more online processors; however I am curious if this would affect machines with only one core.

By passing `const int thread_pool_size = 0` to `CreateDefaultPlatform`
we allow v8 to choose sensible defaults based on the currently online
processors.
@r-52 r-52 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 11, 2015
@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Dec 11, 2015
@bnoordhuis
Copy link
Member

I have no real opinion on whether this is better or worse but for background, the reason I chose 4 is that:

  1. I guesstimated that the average machine has 8 cores, and
  2. libuv's thread pool is also 4 threads big by default.

(4 + 4 technically actually overcommits because of the main thread but a little overcommit seems like a decent trade-off vs. under-utilization.)

One glorious day we'll be able to service libuv and V8 from a single thread pool but until then, we need to strike a balance that doesn't starve one or the other.

Aside, the way to benchmark this is to:

  1. Have a lot of functions that get promoted to the optimized tier around the same time, and
  2. Execute a decent number of (asynchronous) file-based operations, like writing numerous small chunks to a bunch of on-disk files.

Those combined will overload both thread pools to the point that they need to queue up work items and then it's a matter of measuring throughput and latency.

Aside aside, as a stop-gap measure we could tweak node to divide the cores between the two thread pools at start-up.

@jasnell
Copy link
Member

jasnell commented Dec 11, 2015

How aggressive is V8 at consuming the available cores when the default is set to 0? I definitely think that we should look at benchmarking this more before making changes here.

@bnoordhuis
Copy link
Member

How aggressive is V8 at consuming the available cores when the default is set to 0?

It varies. V8 allocates up to three threads for concurrent sweeping; any remaining threads are used for concurrent recompilation and on-stack replacement. It depends on the performance profile of the application whether those threads have work to do.

@jasnell
Copy link
Member

jasnell commented Dec 11, 2015

Would it be possible / desirable to run benchmarks across machines with variable numbers of cores and use that to determine a more algorithmic way of determining the reasonable defaults?

@jasnell
Copy link
Member

jasnell commented Dec 12, 2015

Here's one idea: rather than guessing at this, perhaps add a CLI flag to allow the default to be overridden? e.g. node --v8-default-thread-pool-size=0. The flag would likely not be used extensively so we could just add it to the man page but omit it from the node --help output. Having it would allow someone to go through and experiment with benchmarks using different values to figure out what is optimum.

@tomgco
Copy link
Contributor Author

tomgco commented Dec 12, 2015

Here's one idea: rather than guessing at this, perhaps add a CLI flag to allow the default to be overridden? e.g. node --v8-default-thread-pool-size=0. The flag would likely not be used extensively so we could just add it to the man page but omit it from the node --help output. Having it would allow someone to go through and experiment with benchmarks using different values to figure out what is optimum.

Sounds like a good idea, however I don’t like having undocumented features. Another approach could be to have an --advanced-options flag, much like we currently have for --v8-options and include --v8-default-thread-pool-size=0 under that?

@jasnell
Copy link
Member

jasnell commented Dec 16, 2015

That's certainly a possibility but not sure it's ideal until we have more than one flag that would fall under that kind of category. Adding it to the --help output would be fine.

@tomgco
Copy link
Contributor Author

tomgco commented Dec 18, 2015

Here's one idea: rather than guessing at this, perhaps add a CLI flag to allow the default to be overridden? e.g. node --v8-default-thread-pool-size=0. The flag would likely not be used extensively so we could just add it to the man page but omit it from the node --help output. Having it would allow someone to go through and experiment with benchmarks using different values to figure out what is optimum.

Ok, I am working on this at the moment.

tomgco added a commit to tomgco/node that referenced this pull request Mar 22, 2016
Based on the conversation in nodejs#4243 this implements a way to increase
and decrease the size of the thread pool used in v8.

Currently v8 restricts the thread pool size to `kMaxThreadPoolSize`
which at this commit is (4). So it is only possible to
decrease the thread pool size at the time of this commit. However with
changes upstream this could change at a later date.
If set to 0 then v8 would choose an appropriate size of the thread pool
based on the number of online processors.
jasnell pushed a commit that referenced this pull request Mar 22, 2016
Based on the conversation in #4243 this implements a way to increase
and decrease the size of the thread pool used in v8.

Currently v8 restricts the thread pool size to `kMaxThreadPoolSize`
which at this commit is (4). So it is only possible to
decrease the thread pool size at the time of this commit. However with
changes upstream this could change at a later date.
If set to 0 then v8 would choose an appropriate size of the thread pool
based on the number of online processors.

PR-URL: #4344
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
Based on the conversation in #4243 this implements a way to increase
and decrease the size of the thread pool used in v8.

Currently v8 restricts the thread pool size to `kMaxThreadPoolSize`
which at this commit is (4). So it is only possible to
decrease the thread pool size at the time of this commit. However with
changes upstream this could change at a later date.
If set to 0 then v8 would choose an appropriate size of the thread pool
based on the number of online processors.

PR-URL: #4344
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Based on the conversation in #4243 this implements a way to increase
and decrease the size of the thread pool used in v8.

Currently v8 restricts the thread pool size to `kMaxThreadPoolSize`
which at this commit is (4). So it is only possible to
decrease the thread pool size at the time of this commit. However with
changes upstream this could change at a later date.
If set to 0 then v8 would choose an appropriate size of the thread pool
based on the number of online processors.

PR-URL: #4344
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@fhinkel
Copy link
Member

fhinkel commented Aug 19, 2016

Is this PR still being worked on?

ping @tangxinfa

@bnoordhuis
Copy link
Member

It's been subsumed by PR #4344. I'll close this one.

@bnoordhuis bnoordhuis closed this Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants