Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # 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