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: handling v8 thread pool of zero #42524

Closed
wants to merge 2 commits into from

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Mar 30, 2022

Resolved: #42523

Problem:

If no platform worker exists, Node.js doesn't shut down when background
tasks scheduled exist. It keeps waiting in NodePlatform::DrainTasks.

Observation:

It seems that Node.js used V8's DefaultPlatform implementation a long
time ago, which chooses a suitable default value in case --v8-pool-size=0
is given as a command-line option. However, Node.js currently uses its
own v8::Platform implementation, NodePlatform. It seems not to have
the logic to handle the case.

I referred to #4344 to track the issue.

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.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 30, 2022
Comment on lines +53 to +57
if (uv_cpu_info(&cpu_info, &count) == 0) {
uv_free_cpu_info(cpu_info, count);
thread_pool_size = count - 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 suggest waiting for the upgrade to libuv 1.44 and replacing this with uv_available_parallelism(), it's specifically for use cases such as this one.

uv_cpu_info() isn't really appropriate because it doesn't know how many processors are available to the process, only how many are online.

Copy link
Member Author

Choose a reason for hiding this comment

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

uv_available_parallelism() looks more suited to this case indeed. I'll wait for the upgrade to land.

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++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run worker threads with --v8-pool-size=0
3 participants