src: override v8 thread defaults using cli options #4344

tomgco · 2015-12-18T12:39:43Z

Based on the conversation in #4243 this implements a way to increase
and decrease the size of the thread pool used in v8. Which is defaulted
to 4.

jasnell · 2015-12-18T15:04:29Z

LGTM but definitely want to get @bnoordhuis' opinion /cc @nodejs/ctc

jasnell · 2015-12-18T15:05:03Z

Oh, this would need to be added to the man page also.

tomgco · 2015-12-18T15:12:36Z

Oh, this would need to be added to the man page also.

Thanks for the heads up, I will update this PR with updates to documentation if more “LGTM”’s pour in.

Fishrock123 · 2015-12-18T15:15:05Z

v8 doesn't have a flag for this?

tomgco · 2015-12-18T15:30:54Z

v8 doesn't have a flag for this?

In node.cc we explicitly create a default platform in V8 with a hard-coded value of 4, If set to 0 then v8 would choose an appropriate size of the thread pool based on the number of online processors, in #4243 it was suggested to set a length via CLI options where a suitable default could be decided later on, but at the same time allow to decide based on their own workloads.

But this is an implementation detail, which only embedder’s can change AFAIK.

bnoordhuis · 2015-12-18T15:35:33Z

I checked V8's DefaultPlatform implementation and it actually restricts the thread pool to kMaxThreadPoolSize (4) threads. So with this PR you could lower the number of threads but you can't increase it.

bnoordhuis · 2015-12-18T15:37:00Z

src/node.cc

@@ -3398,6 +3402,8 @@ static void ParseArgs(int* argc,
    } else if (strcmp(arg, "--v8-options") == 0) {
      new_v8_argv[new_v8_argc] = "--help";
      new_v8_argc += 1;
+    } else if (strncmp(arg, "--v8-pool-size=", 15) == 0) {
+      v8_thread_pool_size = arg + 15;


Why not parse the string immediately?

I haven’t written much c++ before, but the rationale behind this is that if we parsed the string immediately we would have to set the value of the int as it is not a pointer it would have to be -1 which I am not sure makes sense?

If it does then I can change it to be -1 if not found and then parse the string immediately.

Couldn't you set the pool size to the default of 4 when it's declared and then update the value here? I think that is what @bnoordhuis is getting at.

@tomgco ... essentially, rather than defining v8_thread_pool_size as a const char*, you can create it as an int with a default value of v8_default_thread_pool_size, then, in the check for --v8-pool-size, parse the string immediately, if it's within the appropriate int range ( >= 0, <= the current maximum ... which is 4) set v8_thread_pool_size to the new value. You can then avoid the pool_size declaration later and simply default_platform = v8::platform::CreateDefaultPlatform(v8_thread_pool_size).

@jasnell Thanks for the hand, I am not going to include the checks that the int's are in an appropriate range as V8 already handles this internally (https://github.com/v8/v8/blob/master/src/libplatform/default-platform.cc#L68-L72), It will save on maintenance time if these values or constants change, do you think that this is a wise thing to do?

tomgco · 2015-12-18T15:57:21Z

I checked V8's DefaultPlatform implementation and it actually restricts the thread pool to kMaxThreadPoolSize (4) threads. So with this PR you could lower the number of threads but you can't increase it.

Thanks, I will make sure to include that in the documentation.

indutny · 2015-12-18T19:04:50Z

Needs docs, otherwise LGTM

Fishrock123 · 2015-12-18T20:01:25Z

src/node.cc

+    pool_size = atoi(v8_thread_pool_size);
+  }
+
+  default_platform = v8::platform::CreateDefaultPlatform(pool_size);


Should we maybe emit warnings here then if the size is too big/small?

i.e. that the threadpool max is 4 if it is larger than that, and also something else if the user specified <1

I would simply say that if the value is not within an acceptable range, the nearest acceptable value is selected... essentially, anything less than 0 is treated as 0, anything greater than the current max is treated as the max. If we add that to the documentation, no warning should be necessary.

I would simply say that if the value is not within an acceptable range, the nearest acceptable value is selected... essentially, anything less than 0 is treated as 0, anything greater than the current max is treated as the max. If we add that to the documentation, no warning should be necessary.

This is already handled in default-platform.cc in V8 https://github.com/v8/v8/blob/master/src/libplatform/default-platform.cc#L68-L72, Any values less than 1 is set to the number of online processors, anything larger than the current kMaxThreadPoolSize is limited to it.

ofrobots · 2015-12-18T22:13:08Z

Thanks, I will make sure to include that in the documentation.

The commit log message also needs to be updated. It would be better not to include specific numbers in the docs, as they may change if V8 changes DefaultPlatform::kMaxThreadPoolSize in the future.

tomgco · 2016-01-08T10:00:45Z

The commit log message also needs to be updated. It would be better not to include specific numbers in the docs, as they may change if V8 changes DefaultPlatform::kMaxThreadPoolSize in the future.

Sorry for the delay in the reply for this, holidays and all, will start fixing all of the issues pointed out now.

tomgco · 2016-01-08T10:50:23Z

@ofrobots @jasnell @Fishrock123 I have authored the commit with a new message, updated the --help output to remove any hard coded values and updated the man page.

ofrobots · 2016-01-08T17:33:49Z

I checked V8's DefaultPlatform implementation and it actually restricts the thread pool to kMaxThreadPoolSize (4) threads. So with this PR you could lower the number of threads but you can't increase it.

FYI, I am trying to fix this limitation upstream in this CL: https://codereview.chromium.org/1553653002/

jasnell · 2016-01-15T01:18:02Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/1244/

jasnell · 2016-01-15T01:20:01Z

@tomgco ... ping.. have you been able to look at the most recently comments...

tomgco · 2016-01-22T11:10:07Z

@tomgco ... ping.. have you been able to look at the most recently comments...

Sorry about the delay, I have been without Internet. I am looking at them now. Thanks for the reminder.

tomgco · 2016-01-22T11:53:06Z

@ofrobots @jasnell @Fishrock123 I have addressed all comments and updated the commit as well as the wording of the commit message.

cjihrig · 2016-01-22T15:32:39Z

src/node.cc

@@ -168,6 +169,7 @@ static uv_async_t dispatch_debug_messages_async;
 static node::atomic<Isolate*> node_isolate;
 static v8::Platform* default_platform;

+int v8_thread_pool_size = v8_default_thread_pool_size;


Would it work to ditch v8_default_thread_pool_size, and just initialize this to DefaultPlatform::kMaxThreadPoolSize, like @ofrobots suggested?

I think you'll want to group this variable with the variables declared starting at line 134.

Would it work to ditch v8_default_thread_pool_size, and just initialize this to DefaultPlatform::kMaxThreadPoolSize, like @ofrobots suggested?

That would depend if we wanted to change our defaults based on upstream v8 changes, or have a default value that is controlled from within core. Let's say v8 changed kMaxThreadPoolSize to MAX_INT, I am sure that we would not want to set the default based on this?

I think you'll want to group this variable with the variables declared starting at line 134.

Thanks! Was looking for a nicer place. Will update that now.

Good point. Leaving it as is is fine.

tomgco · 2016-01-22T16:12:52Z

@cjihrig Ok, pushed the grouping of the variables.

cjihrig · 2016-01-22T16:30:12Z

src/node.cc

@@ -144,6 +144,8 @@ static const char** preload_modules = nullptr;
 static bool use_debug_agent = false;
 static bool debug_wait_connect = false;
 static int debug_port = 5858;
+static const int v8_default_thread_pool_size = 4;
+int v8_thread_pool_size = v8_default_thread_pool_size;


Seems like this should probably be static for consistency.

cjihrig · 2016-01-22T16:30:33Z

LGTM. It would be nice if there were a way to test this though.

jasnell · 2016-01-23T02:15:14Z

LGTM, can't think of a reliable way of testing tho

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 <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2016-03-22T14:57:07Z

Landed in 03e07d3

Fishrock123 · 2016-03-22T19:13:35Z

I'm a little concerned that the flag is not descriptive enough and should be renamed to --v8-thread-pool-size before we have a release with it. Thoughts?

jasnell · 2016-03-22T19:27:15Z

sounds ok to me. I'm good either way tho.

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 <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. (Forrest L Norvell) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344) PR-URL: #5970

fix: nodejs#42523 Problem: If no platform worker exists, Node.js doesn't shut down when background tasks exist. It keeps waiting in `NodePlatform::DrainTasks`. Observation: It seems that Node.js used to use V8's `DefaultPlatform` implementation, which chooses a suitable default value in case that `--v8-pool-size=0` is given as a command-line option. However, Node.js currently uses its own v8::Platform implementation, `NodePlatform`. It doesn't have the logic to handle the case. I referred to nodejs#4344 to track the issue.

jasnell added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 18, 2015

bnoordhuis reviewed Dec 18, 2015
View reviewed changes

Fishrock123 reviewed Dec 18, 2015
View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 24, 2015

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

tomgco force-pushed the overrideable-v8-thread-pool branch from a5f04b4 to 1bb264e Compare January 8, 2016 10:45

tomgco force-pushed the overrideable-v8-thread-pool branch from 1bb264e to f4ebbec Compare January 22, 2016 11:48

cjihrig reviewed Jan 22, 2016
View reviewed changes

tomgco force-pushed the overrideable-v8-thread-pool branch from f4ebbec to 8d835e4 Compare January 22, 2016 16:11

cjihrig reviewed Jan 22, 2016
View reviewed changes

tomgco force-pushed the overrideable-v8-thread-pool branch from 8d835e4 to e38a8be Compare January 22, 2016 17:03

jasnell closed this Mar 22, 2016

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

bnoordhuis mentioned this pull request Aug 19, 2016

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

Closed

snyk-bot mentioned this pull request Feb 12, 2022

[Snyk] Upgrade rollup from 2.58.0 to 2.65.0 Iceymann18777/node#12

Open

daeyeon mentioned this pull request Mar 30, 2022

src: handling v8 thread pool of zero #42524

Closed

leonardoadame mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015

Open

leonardoadame mentioned this pull request Feb 11, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 leonardoadame/node#646

Open

aliscco mentioned this pull request Feb 12, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 aliscco/alisco-node#1250

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: override v8 thread defaults using cli options #4344

src: override v8 thread defaults using cli options #4344

tomgco commented Dec 18, 2015

jasnell commented Dec 18, 2015

jasnell commented Dec 18, 2015

tomgco commented Dec 18, 2015

Fishrock123 commented Dec 18, 2015

tomgco commented Dec 18, 2015

bnoordhuis commented Dec 18, 2015

bnoordhuis Dec 18, 2015

tomgco Dec 18, 2015

cjihrig Jan 8, 2016

jasnell Jan 8, 2016

tomgco Jan 22, 2016

tomgco commented Dec 18, 2015

indutny commented Dec 18, 2015

Fishrock123 Dec 18, 2015

jasnell Jan 8, 2016

tomgco Jan 22, 2016

ofrobots commented Dec 18, 2015

tomgco commented Jan 8, 2016

tomgco commented Jan 8, 2016

ofrobots commented Jan 8, 2016

jasnell commented Jan 15, 2016

jasnell commented Jan 15, 2016

tomgco commented Jan 22, 2016

tomgco commented Jan 22, 2016

cjihrig Jan 22, 2016

tomgco Jan 22, 2016

cjihrig Jan 22, 2016

tomgco commented Jan 22, 2016

cjihrig Jan 22, 2016

cjihrig commented Jan 22, 2016

jasnell commented Jan 23, 2016

jasnell commented Mar 22, 2016

Fishrock123 commented Mar 22, 2016

jasnell commented Mar 22, 2016

src: override v8 thread defaults using cli options #4344

src: override v8 thread defaults using cli options #4344

Conversation

tomgco commented Dec 18, 2015

jasnell commented Dec 18, 2015

jasnell commented Dec 18, 2015

tomgco commented Dec 18, 2015

Fishrock123 commented Dec 18, 2015

tomgco commented Dec 18, 2015

bnoordhuis commented Dec 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomgco commented Dec 18, 2015

indutny commented Dec 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofrobots commented Dec 18, 2015

tomgco commented Jan 8, 2016

tomgco commented Jan 8, 2016

ofrobots commented Jan 8, 2016

jasnell commented Jan 15, 2016

jasnell commented Jan 15, 2016

tomgco commented Jan 22, 2016

tomgco commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomgco commented Jan 22, 2016

Choose a reason for hiding this comment

cjihrig commented Jan 22, 2016

jasnell commented Jan 23, 2016

jasnell commented Mar 22, 2016

Fishrock123 commented Mar 22, 2016

jasnell commented Mar 22, 2016