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

Use Runtime#availableProcessors on Linux #16512

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Oct 19, 2022

As of JDK 11, Runtime#availableProcessors on Linux is aware of cgroup resource limits and thus behaves as expected in containers, whereas /proc/cpuinfo still reports the number of logical processors available to the host.

Unfortunately, the JVM-internal os::total_memory function, which similarly takes cgroup limits into account when computing the available RAM, does not seem to be accessible from Java.

https://hg.openjdk.java.net/jdk/hs/rev/7f22774a5f42#l6.178

Fixes #5042

As of JDK 11, `Runtime#availableProcessors` on Linux is aware of cgroup
resource limits and thus behaves as expected in containers, whereas
`/proc/cpuinfo` still reports the number of logical processors available
to the host.

Unfortunately, the JVM-internal `os::total_memory` function, which
similarly takes cgroup limits into account when computing the available
RAM, does not seem to be accessible from Java.

https://hg.openjdk.java.net/jdk/hs/rev/7f22774a5f42#l6.178
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 19, 2022

@philwo I just followed the observation you made in #3886 (comment). Can you take a look?

As described in the PR description, it doesn't seem possible to get the amount of physical memory this way.

@fmeum fmeum marked this pull request as ready for review October 19, 2022 21:39
@sgowroji sgowroji added team-Local-Exec Issues and PRs for the Execution (Local) team awaiting-review PR is awaiting review from an assigned reviewer labels Oct 20, 2022
Copy link
Member

@philwo philwo left a comment

Choose a reason for hiding this comment

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

Very nice, thank you! :)

@meisterT Could you assign this to someone in your team to review & merge it?

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 20, 2022

@meisterT Thanks for the approval. Doesn't this have to be labeled with "awaiting-merge" though? It still says "awaiting-review".

@meisterT
Copy link
Member

@fmeum I already started the import yesterday, but some internal tests ran into timeouts and I have to figure out whether that is related or not.

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 21, 2022

Thanks for working on that! Just didn't want it to get lost as I thought that label was required.

@meisterT
Copy link
Member

Update: that specific test needs at least 3 loading phase threads to run correctly and we were running it in a constrained container with just 2 cores.

While the test can be fixed, it points to a deeper problem here: we automatically deduce the number of loading phase threads (see https://cs.opensource.google/bazel/bazel/+/0232dd7cd18bcf807701a5b81367ed794b1fc357:src/main/java/com/google/devtools/build/lib/runtime/LoadingPhaseThreadsOption.java;l=41) based on the host resources available.

Thus, this change can make quite a difference in how Bazel performs in container environments. I would prefer guarding it behind an incompatible flag, so we have a less risky way of rolling this out.

Copy link
Contributor

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

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

This is wholesome, hope it lands in Bazel 6, we run Bazel CI in k8s with CPU limits and this will finally let Bazel know the actual CPU limits in a container it runs in rather than hardware/virtual_machine CPU limits.

@meisterT meisterT added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 14, 2022
@meisterT
Copy link
Member

@artem-zinnatullin It is very very unlikely that this will land in Bazel 6 given the comment I wrote above. We need to guard it behind a flag.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 17, 2022

We discussed offline that guarding this behind a flag is difficult because it is used during option parsing (providing default values). It would probably need to be a startup option or JVM property.

Thinking more about this, wouldn't users who are potentially broken by this change be able to supply --local_cpu_resources and --loading_phase_threads manually?

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 7, 2023

@meisterT Friendly ping, which approach do you prefer:

  1. startup flag
  2. JVM property (settable via --host_jvm_args)
  3. no flag, but users can use --local_cpu_resources and --loading_phase_threads to override

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 22, 2023

@meisterT Friendly ping

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Feb 24, 2023
@copybara-service copybara-service bot closed this in ab2953c Mar 8, 2023
@meisterT
Copy link
Member

meisterT commented Mar 8, 2023

@fmeum sorry for the delay, it is merged now.

I added RELNOTES[INC] to the commit description (see ab2953c) and think that this is the most reasonable path forward.

fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
As of JDK 11, `Runtime#availableProcessors` on Linux is aware of cgroup resource limits and thus behaves as expected in containers, whereas `/proc/cpuinfo` still reports the number of logical processors available to the host.

Unfortunately, the JVM-internal `os::total_memory` function, which similarly takes cgroup limits into account when computing the available RAM, does not seem to be accessible from Java.

https://hg.openjdk.java.net/jdk/hs/rev/7f22774a5f42#l6.178

Fixes bazelbuild#5042

Closes bazelbuild#16512.

RELNOTES[INC]: Bazel's local CPU resource on Linux is now container aware. Use `--local_cpu_resources`, `--loading_phase_threads` or `--jobs` to override.

PiperOrigin-RevId: 515032721
Change-Id: I103ee25920d6908545ce6bf03d9c42c604c8589b
copybara-service bot pushed a commit that referenced this pull request Dec 7, 2023
As of JDK 14, `OperatingSystemMXBean` provides information about system memory that is container-aware. Outside containers, it uses the same mechanisms as Bazel to determine available RAM (`/proc/meminfo` on Linux, `hw.memsize` on macOS) and can thus be used as a drop-in replacement for the custom implementation.

A small caveat is that Bazel's macOS RAM estimate was based on converting bytes to "MB" via a divisor of `1000^2` instead of `1024^2`, resulting in a consistent overestimate compared to an identical Linux machine that is now corrected.

This opportunity was missed in #16512 since `OperatingSystemMXBean` is based on a complete Java implementation of cgroups handling and doesn't go through the `os::total_memory` or `os::physical_memory` Hotspot functions.

RELNOTES[INC]:
* On Linux, Bazel's RAM estimate for the host machine is now aware of container resource limits.
* On macOS, Bazel no longer consistently overestimates the total RAM by ~5% (`1024^2/1000^2`).
* On Windows, Bazel's RAM estimate is now generally more accurate as it is no longer influenced by JVM heuristics.

Fixes #3886

Closes #20435.

PiperOrigin-RevId: 588718034
Change-Id: I2daafa0567740a1b149ca8756ec27f102129283c
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 21, 2023
As of JDK 14, `OperatingSystemMXBean` provides information about system memory that is container-aware. Outside containers, it uses the same mechanisms as Bazel to determine available RAM (`/proc/meminfo` on Linux, `hw.memsize` on macOS) and can thus be used as a drop-in replacement for the custom implementation.

A small caveat is that Bazel's macOS RAM estimate was based on converting bytes to "MB" via a divisor of `1000^2` instead of `1024^2`, resulting in a consistent overestimate compared to an identical Linux machine that is now corrected.

This opportunity was missed in bazelbuild#16512 since `OperatingSystemMXBean` is based on a complete Java implementation of cgroups handling and doesn't go through the `os::total_memory` or `os::physical_memory` Hotspot functions.

RELNOTES[INC]:
* On Linux, Bazel's RAM estimate for the host machine is now aware of container resource limits.
* On macOS, Bazel no longer consistently overestimates the total RAM by ~5% (`1024^2/1000^2`).
* On Windows, Bazel's RAM estimate is now generally more accurate as it is no longer influenced by JVM heuristics.

Fixes bazelbuild#3886

Closes bazelbuild#20435.

PiperOrigin-RevId: 588718034
Change-Id: I2daafa0567740a1b149ca8756ec27f102129283c
github-merge-queue bot pushed a commit that referenced this pull request Jan 11, 2024
As of JDK 14, `OperatingSystemMXBean` provides information about system
memory that is container-aware. Outside containers, it uses the same
mechanisms as Bazel to determine available RAM (`/proc/meminfo` on
Linux, `hw.memsize` on macOS) and can thus be used as a drop-in
replacement for the custom implementation.

A small caveat is that Bazel's macOS RAM estimate was based on
converting bytes to "MB" via a divisor of `1000^2` instead of `1024^2`,
resulting in a consistent overestimate compared to an identical Linux
machine that is now corrected.

This opportunity was missed in
#16512 since
`OperatingSystemMXBean` is based on a complete Java implementation of
cgroups handling and doesn't go through the `os::total_memory` or
`os::physical_memory` Hotspot functions.

RELNOTES[INC]:
* On Linux, Bazel's RAM estimate for the host machine is now aware of
container resource limits.
* On macOS, Bazel no longer consistently overestimates the total RAM by
~5% (`1024^2/1000^2`).
* On Windows, Bazel's RAM estimate is now generally more accurate as it
is no longer influenced by JVM heuristics.

Fixes #3886

Closes #20435.

Commit
2f3cdc5

PiperOrigin-RevId: 588718034
Change-Id: I2daafa0567740a1b149ca8756ec27f102129283c

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
@fmeum fmeum deleted the 5042-respect-cpu-cgroups branch June 5, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make bazel cpu counter container aware
5 participants