GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: override v8 thread defaults using cli options by tomgco · Pull Request #4344 · nodejs/node

Conversation

@tomgco

@tomgco tomgco commented Dec 18, 2015

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Dec 18, 2015

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Dec 18, 2015
@jasnell

jasnell commented Dec 18, 2015

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomgco

tomgco commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Dec 18, 2015

Copy link
Copy Markdown
Contributor

v8 doesn't have a flag for this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomgco

tomgco commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Dec 18, 2015

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Dec 18, 2015
View reviewed changes
Comment thread src/node.cc Outdated

@bnoordhuis bnoordhuis Dec 18, 2015

Copy link
Copy Markdown
Member

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.

Why not parse the string immediately?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tomgco tomgco Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Jan 8, 2016

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Jan 8, 2016

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tomgco tomgco Jan 22, 2016

Copy link
Copy Markdown
Contributor Author

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.

@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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tomgco

tomgco commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Dec 18, 2015

Copy link
Copy Markdown
Member

Needs docs, otherwise LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed Dec 18, 2015
View reviewed changes
Comment thread src/node.cc Outdated

@Fishrock123 Fishrock123 Dec 18, 2015

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Jan 8, 2016

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tomgco tomgco Jan 22, 2016

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ofrobots

ofrobots commented Dec 18, 2015

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 24, 2015
@Trott
Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01
@tomgco

tomgco commented Jan 8, 2016

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomgco
tomgco force-pushed the overrideable-v8-thread-pool branch from a5f04b4 to 1bb264e Compare January 8, 2016 10:45
@tomgco

tomgco commented Jan 8, 2016

Copy link
Copy Markdown
Contributor Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ofrobots

ofrobots commented Jan 8, 2016

Copy link
Copy Markdown
Contributor

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 15, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 15, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomgco

tomgco commented Jan 22, 2016

Copy link
Copy Markdown
Contributor Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomgco
tomgco force-pushed the overrideable-v8-thread-pool branch from 1bb264e to f4ebbec Compare January 22, 2016 11:48
@tomgco

tomgco commented Jan 22, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed Jan 22, 2016
View reviewed changes
Comment thread src/node.cc Outdated

@cjihrig cjihrig Jan 22, 2016

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tomgco tomgco Jan 22, 2016

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Jan 22, 2016

Copy link
Copy Markdown
Contributor

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.

Good point. Leaving it as is is fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tomgco
tomgco force-pushed the overrideable-v8-thread-pool branch from f4ebbec to 8d835e4 Compare January 22, 2016 16:11
@tomgco

tomgco commented Jan 22, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig Ok, pushed the grouping of the variables.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed Jan 22, 2016
View reviewed changes
Comment thread src/node.cc Outdated

@cjihrig cjihrig Jan 22, 2016

Copy link
Copy Markdown
Contributor

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.

Seems like this should probably be static for consistency.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@cjihrig

cjihrig commented Jan 22, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomgco
tomgco force-pushed the overrideable-v8-thread-pool branch from 8d835e4 to e38a8be Compare January 22, 2016 17:03
@jasnell

jasnell commented Jan 23, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell pushed a commit that referenced this pull request Mar 22, 2016
@tomgco @jasnell
src: override v8 thread defaults using cli options
03e07d3 
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]>
@jasnell

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

Landed in 03e07d3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Mar 22, 2016
@Fishrock123

Fishrock123 commented Mar 22, 2016

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
@tomgco @evanlucas
src: override v8 thread defaults using cli options
3f6fbaf 
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 evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@tomgco @evanlucas
src: override v8 thread defaults using cli options
0d0c57f 
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 added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
9cd6e98 
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
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
1bb2c4b 
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
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
79ea8c3 
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
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
64c87e2 
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
@bnoordhuis bnoordhuis mentioned this pull request Aug 19, 2016
src: use thread defaults provided by v8::platform #4243
Closed
@snyk-bot 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 added a commit to daeyeon/node that referenced this pull request Mar 30, 2022
@daeyeon
src: handling v8 thread pool of zero
9fd2df7 
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 daeyeon mentioned this pull request Mar 30, 2022
src: handling v8 thread pool of zero #42524
Closed
daeyeon added a commit to daeyeon/node that referenced this pull request Apr 22, 2022
@daeyeon
src: handling v8 thread pool of zero
3c5d0a3 
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.
@leonardoadame leonardoadame mentioned this pull request Dec 1, 2023
[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377
Open
@aliscco aliscco mentioned this pull request Dec 2, 2023
[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015
Open
@leonardoadame 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@tomgco @jasnell @Fishrock123 @bnoordhuis @indutny @ofrobots @cjihrig @r-52
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.