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: override v8 thread defaults using cli options #4344

Closed
wants to merge 1 commit into from

Conversation

tomgco
Copy link
Contributor

@tomgco tomgco commented Dec 18, 2015

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

jasnell commented Dec 18, 2015

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

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

jasnell commented Dec 18, 2015

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

@tomgco
Copy link
Contributor Author

tomgco commented Dec 18, 2015

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

v8 doesn't have a flag for this?

@tomgco
Copy link
Contributor Author

tomgco commented Dec 18, 2015

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

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.

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

Choose a reason for hiding this comment

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

Why not parse the string immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

tomgco commented Dec 18, 2015

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

indutny commented Dec 18, 2015

Needs docs, otherwise LGTM

pool_size = atoi(v8_thread_pool_size);
}

default_platform = v8::platform::CreateDefaultPlatform(pool_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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.

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

tomgco commented Jan 8, 2016

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 tomgco force-pushed the overrideable-v8-thread-pool branch from a5f04b4 to 1bb264e Compare January 8, 2016 10:45
@tomgco
Copy link
Contributor Author

tomgco commented Jan 8, 2016

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

ofrobots commented Jan 8, 2016

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

jasnell commented Jan 15, 2016

@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

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

@tomgco
Copy link
Contributor Author

tomgco commented Jan 22, 2016

@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 tomgco force-pushed the overrideable-v8-thread-pool branch from 1bb264e to f4ebbec Compare January 22, 2016 11:48
@tomgco
Copy link
Contributor Author

tomgco commented Jan 22, 2016

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Leaving it as is is fine.

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

tomgco commented Jan 22, 2016

@cjihrig Ok, pushed the grouping of the variables.

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

Choose a reason for hiding this comment

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

Seems like this should probably be static for consistency.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 22, 2016

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

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

jasnell commented Jan 23, 2016

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

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

jasnell commented Mar 22, 2016

Landed in 03e07d3

@jasnell jasnell closed this Mar 22, 2016
@Fishrock123
Copy link
Contributor

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

jasnell commented Mar 22, 2016

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

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 <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas added a commit that referenced this pull request Mar 31, 2016
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)
evanlucas added a commit that referenced this pull request Mar 31, 2016
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)
evanlucas added a commit that referenced this pull request Apr 1, 2016
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)
evanlucas added a commit that referenced this pull request Apr 1, 2016
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
daeyeon added a commit to daeyeon/node that referenced this pull request Mar 30, 2022
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.
daeyeon added a commit to daeyeon/node that referenced this pull request Apr 14, 2022
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.
daeyeon added a commit to daeyeon/node that referenced this pull request Apr 22, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants