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

os: add availableParallelism() #45895

Merged
merged 2 commits into from
Dec 21, 2022
Merged

os: add availableParallelism() #45895

merged 2 commits into from
Dec 21, 2022

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 17, 2022

This commit exposes uv_available_parallelism() as an alternative to cpus().length. uv_available_parallelism() is inspired by Rust's available_parallelism().

@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. os Issues and PRs related to the os subsystem. labels Dec 17, 2022
doc/api/os.md Show resolved Hide resolved
@cjihrig cjihrig removed the needs-ci PRs that need a full CI run. label Dec 17, 2022
@nodejs-github-bot

This comment was marked as outdated.

@cjihrig cjihrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2022
doc/api/os.md Outdated Show resolved Hide resolved
src/node_os.cc Show resolved Hide resolved
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

* Returns: {integer}

Returns an estimate of the default amount of parallelism a program should use.
Always returns a value greater than zero.
Copy link
Member

Choose a reason for hiding this comment

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

Some explanation about how that estimate is reached would be helpful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the PR that added this functionality to libuv - libuv/libuv@f250c6c. If you have some exact text you'd like to see based on that, I'm happy to add it. The CI has been so flaky (and frustrating) that I don't want to go back and forth with pushing changes, running the CI, making more changes, etc. - I'd rather just ship whatever suggestions you have.

Alternatively, a docs update could be added after the fact since the CI requirements for docs only changes are much lower.

Copy link
Member

Choose a reason for hiding this comment

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

The implementation of uv_available_parallelism() will likely evolve over time so the docs here shouldn't be too specific, or they'll become outdated.

My suggestions are to a) link to the libuv documentation, b) leave it out, or c) say we consult the animating spirits inside the computer.

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'd like to see how option C plays out.

Copy link
Member

Choose a reason for hiding this comment

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

Returns an estimate of the default amount of parallelism a program should use. Always returns a value greater than zero.

On Linux, inspects the calling thread’s CPU affinity mask to determine if it has been pinned to specific CPUs.

On Windows, the available parallelism may be underreported on systems with more than 64 logical CPUs.

On other platforms, reports the number of CPUs that the operating system considers to be online.

The heuristics used are expected to evolve over time and may vary across operating systems.

Or, if nothing else, link to the libuv documentation.

Choose a reason for hiding this comment

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

Does not answer the question of what it does inside of a cgroup, which is the primary problem with cpus()

Copy link
Member

Choose a reason for hiding this comment

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

It's not mentioned because it's oblivious to the presence of cgroups.

Copy link

@jdmarshall jdmarshall Jan 6, 2023

Choose a reason for hiding this comment

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

I'm not a fan of adding new features that replicate open bugs in existing functionality.

Also #28855 specifically asks for this problem (cgoups causing process count to be wrong) to be fixed, which is what my question is asking. Obliviousness is exactly the problem. cpus().length is oblivious to cgroups and gives the processor count of the host, not what's available to the container.

If the answer is 'no', we still don't have a solution to the issue that was marked closed.

Copy link

@jdmarshall jdmarshall Jan 6, 2023

Choose a reason for hiding this comment

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

I'm lousy at reading C code, but I think this is saying:

libuv/libuv@f250c6c
https://www.cygwin.com/bugzilla/show_bug.cgi?id=27645

That sysconf() works on musl (eg, Alpine Linux?) but not on glibc (eg, Ubuntu, CoreOS). But the extra call in the conditional block in the libuv commit above emulates the musl behavior. If I've followed that right, then the answer is 'yes' and it's not oblivious to cgroups.

Copy link
Member

Choose a reason for hiding this comment

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

It's oblivious in the "cache oblivious algorithm" sense: libuv doesn't have to care whether cgroups are active or not; if the container is set up properly, it'll produce the right answer.

@jasnell
Copy link
Member

jasnell commented Dec 19, 2022

LGTM but I would like to see the doc comment addressed.

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 19, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2022

RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
With the introduction of os.availableParallelism(), users should
no longer rely on os.cpus().length to determine the amount of
available parallelism. This commit adds a note to the os.cpus()
docs.

PR-URL: #45895
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
Refs: #45895
PR-URL: #45979
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 5, 2023
Notable changes:

buffer:
  * (SEMVER-MINOR) add buffer.isUtf8 for utf8 validation (Yagiz Nizipli) #45947
http:
  * (SEMVER-MINOR) improved timeout defaults handling (Paolo Insogna) #45778
net
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
os:
  * (SEMVER-MINOR) add availableParallelism() (Colin Ihrig) #45895
util:
  * add fast path for text-decoder fatal flag (Yagiz Nizipli) #45803

PR-URL: #46061
RafaelGSS added a commit that referenced this pull request Jan 5, 2023
Notable changes:

buffer:
  * (SEMVER-MINOR) add buffer.isUtf8 for utf8 validation (Yagiz Nizipli) #45947
http:
  * (SEMVER-MINOR) improved timeout defaults handling (Paolo Insogna) #45778
net
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
os:
  * (SEMVER-MINOR) add availableParallelism() (Colin Ihrig) #45895
util:
  * add fast path for text-decoder fatal flag (Yagiz Nizipli) #45803

PR-URL: #46061
RafaelGSS added a commit that referenced this pull request Jan 6, 2023
Notable changes:

buffer:
  * (SEMVER-MINOR) add buffer.isUtf8 for utf8 validation (Yagiz Nizipli) #45947
http:
  * (SEMVER-MINOR) improved timeout defaults handling (Paolo Insogna) #45778
net
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
os:
  * (SEMVER-MINOR) add availableParallelism() (Colin Ihrig) #45895
util:
  * add fast path for text-decoder fatal flag (Yagiz Nizipli) #45803

PR-URL: #46061
nodejs-github-bot pushed a commit that referenced this pull request Jan 11, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jan 11, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
Refs: nodejs#45895
PR-URL: nodejs#46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
Refs: nodejs#45895
PR-URL: nodejs#46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
This commit exposes uv_available_parallelism() as an alternative
to cpus().length. uv_available_parallelism() is inspired by
Rust's available_parallelism().

PR-URL: #45895
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
With the introduction of os.availableParallelism(), users should
no longer rely on os.cpus().length to determine the amount of
available parallelism. This commit adds a note to the os.cpus()
docs.

PR-URL: #45895
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
Refs: #45895
PR-URL: #45979
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
This commit exposes uv_available_parallelism() as an alternative
to cpus().length. uv_available_parallelism() is inspired by
Rust's available_parallelism().

PR-URL: #45895
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
With the introduction of os.availableParallelism(), users should
no longer rely on os.cpus().length to determine the amount of
available parallelism. This commit adds a note to the os.cpus()
docs.

PR-URL: #45895
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
Refs: #45895
PR-URL: #45979
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
Refs: #45895
PR-URL: #46003
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
juanarbol added a commit that referenced this pull request Feb 1, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
juanarbol added a commit that referenced this pull request Feb 1, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
juanarbol added a commit that referenced this pull request Feb 1, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
juanarbol added a commit that referenced this pull request Feb 2, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. os Issues and PRs related to the os subsystem. 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.

10 participants