Generalize memory and cpu tracking #19839

AlessandroPatti · 2023-10-17T08:50:17Z

Remove special handling of cpu and memory, and instead track them as any other resource. This has a few advantages:

Significant code clean up, keep only the logic for generic resource around
Consistent resources type (double) for all resource
Consistent resource tag for all resources: resources:memory:<n> works

Relates to #19679

zhengwei143 · 2023-11-13T17:12:34Z

Hiya, I'm handling the review but not too familiar with this area of the code yet, will spend some time looking at this and will get a review out by this week.

zhengwei143 · 2023-11-16T16:32:28Z

src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java

@@ -37,19 +37,13 @@
 public class ResourceSet implements ResourceSetOrBuilder {


Can we consolidate "memory" and "cpu" string usage as a static string defined here?

zhengwei143 · 2023-11-16T16:49:45Z

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

+    ImmutableMap<String, Double> cpuRam =
+            ImmutableMap.of("cpu", options.localCpuResources, "memory", options.localRamResources);
+    ImmutableMap<String, Double> resources =
+       java.util.stream.Stream.concat(cpuRam.entrySet().stream(), options.localExtraResources.stream())


Please import java.util.stream.Stream above and use Stream.concat() instead.

zhengwei143 · 2023-11-16T17:05:52Z

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

+    ImmutableMap<String, Double> cpuRam =
+            ImmutableMap.of("cpu", options.localCpuResources, "memory", options.localRamResources);
+    ImmutableMap<String, Double> resources =
+       java.util.stream.Stream.concat(cpuRam.entrySet().stream(), options.localExtraResources.stream())
            .collect(
                ImmutableMap.toImmutableMap(
                    Map.Entry::getKey, Map.Entry::getValue, (v1, v2) -> v2));


IIUC, users will be able to override --local_{cpu,ram}_resources here because there isn't a check to ensure that users don't specify --local_extra_resources={cpu,memory}=?. Because localExtraResources.stream() is concatenated after cpuRam.stream() and are collected into a single map here, the former overrides the latter.

Since this map is now the source of truth (i.e. cpu and memory don't have separate fields in the ResourceSet), Bazel won't ignore any "cpu" or "memory" keys in the map as it previously did.

Lets keep the previous behavior of still having the --local_{cpu,ram}_resources flag (at least in this change). So you'd want to either flip the order of the concat, or filter out "cpu" and "memory" strings from localExtraResources.

Updated. I was a bit on the fence on what was best when I first made this change, because ideally I'd be nice to have one attribute to deal with all resources (see proposal in #19679). I do agree however that --local_extra_resources still carries that meaning of "resources other than some others". Might be worth adopting an incompatible flag to introduce --local_resources, WDYT? Happy to do it as a follow up as well.

I agree, let's follow up with a separate PR (feel free to send it to me directly). --local_resources wouldn't need to be an incompatible flag; we can just have it override all of --local_{cpu,ram,extra}_resources when specified, then we can mark the old flags for deprecation and remove them in the following major release.

zhengwei143 · 2023-11-16T17:09:10Z

src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java

-        + cpuUsage
-        + "\n"
-        + Joiner.on("\n").withKeyValueSeparator(": ").join(extraResourceUsage.entrySet())
+        + Joiner.on("\n").withKeyValueSeparator(": ").join(resources.entrySet())


Can you preserve the behavior of this? I think it'd be better to show Memory and CPU at the top since they are the more typical resources, then the rest of the resources below.

zhengwei143 · 2023-11-16T17:14:04Z

src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java

+      // requested resource sets use pessimistic estimations. Note that this
+      // ratio is used only during comparison - for tracking we will actually
+      // mark whole requested amount as used.
+      double requested = resource.getValue() * MIN_NECESSARY_RATIO.getOrDefault(key, 1.0);


Could you define 1.0 in a static constant above together with MIN_NECESSARY_RATIO (e.g. DEFAULT_RATIO)? That'll make it clearer what happens if it's not specified in the MIN_NECESSARY_RATIO map.

zhengwei143

Thanks for your contribution! I'll get this imported internally ASAP.

fmeum · 2023-11-21T11:01:10Z

@bazel-io flag

zhengwei143 · 2023-11-22T08:52:00Z

Update: It's failing some internal tests that I'll have to spend some time fixing

brentleyjones · 2023-11-28T13:31:10Z

@bazel-io flag

keertk · 2023-11-28T15:08:32Z

@brentleyjones @fmeum is this flagged for 7.0? Or 7.1?

fmeum · 2023-11-28T15:21:12Z

@keertk I would say 7.1.0 at this point.

keertk · 2023-11-28T15:22:15Z

@bazel-io fork 7.1.0

AlessandroPatti · 2023-11-30T18:06:44Z

@keertk @fmeum Looks like 7.0 was delayd again #18548 (comment), maybe there's still a chance for this to go in?

keertk · 2023-11-30T18:10:16Z

@AlessandroPatti we're actually planning on getting the final RC out in a little bit, but we can try to include this.

Remove special handling of cpu and memory, and instead track them as any other resource. This has a few advantages: - Significant code clean up, keep only the logic for generic resource around - Consistent resources type (double) for all resource Relates to bazelbuild#19679 Closes bazelbuild#19839. PiperOrigin-RevId: 585926931 Change-Id: Id0d6a14e9c151c3895f55d1808b6bd66eecf98b3

Remove special handling of cpu and memory, and instead track them as any other resource. This has a few advantages: - Significant code clean up, keep only the logic for generic resource around - Consistent resources type (double) for all resource Relates to #19679 Closes #19839. Commit ebe4d0d PiperOrigin-RevId: 585926931 Change-Id: Id0d6a14e9c151c3895f55d1808b6bd66eecf98b3 Co-authored-by: Alessandro Patti <ale812@yahoo.it>

iancha1992 · 2023-12-01T19:29:02Z

The changes in this PR have been included in Bazel 7.0.0 RC5. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

Follow up for #19839 (comment), introduce new flag that will replace all the others. Closes #20398. PiperOrigin-RevId: 596905096 Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52

Follow up for bazelbuild#19839 (comment), introduce new flag that will replace all the others. Closes bazelbuild#20398. PiperOrigin-RevId: 596905096 Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52

Follow up for #19839 (comment), introduce new flag that will replace all the others. Closes #20398. Commit ea75928 PiperOrigin-RevId: 596905096 Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52 Co-authored-by: Alessandro Patti <ale812@yahoo.it>

Remove special handling of cpu and memory, and instead track them as any other resource. This has a few advantages: - Significant code clean up, keep only the logic for generic resource around - Consistent resources type (double) for all resource Relates to bazelbuild#19679 Closes bazelbuild#19839. PiperOrigin-RevId: 585926931 Change-Id: Id0d6a14e9c151c3895f55d1808b6bd66eecf98b3

Follow up for bazelbuild#19839 (comment), introduce new flag that will replace all the others. Closes bazelbuild#20398. PiperOrigin-RevId: 596905096 Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52

Remove special handling of cpu and memory, and instead track them as any other resource. This has a few advantages: - Significant code clean up, keep only the logic for generic resource around - Consistent resources type (double) for all resource Relates to bazelbuild#19679 Closes bazelbuild#19839. PiperOrigin-RevId: 585926931 Change-Id: Id0d6a14e9c151c3895f55d1808b6bd66eecf98b3

Follow up for bazelbuild#19839 (comment), introduce new flag that will replace all the others. Closes bazelbuild#20398. PiperOrigin-RevId: 596905096 Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52

AlessandroPatti requested a review from a team as a code owner October 17, 2023 08:50

AlessandroPatti requested review from gregestren and removed request for a team October 17, 2023 08:50

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions team-Local-Exec Issues and PRs for the Execution (Local) team team-Performance Issues for Performance teams labels Oct 17, 2023

AlessandroPatti force-pushed the apatti/19679/general branch 2 times, most recently from 5c9ac6d to 76f6134 Compare October 17, 2023 09:21

[Resources] Generalize memory and cpu

3cca2a7

AlessandroPatti force-pushed the apatti/19679/general branch from 76f6134 to 3cca2a7 Compare October 17, 2023 10:34

meisterT requested a review from zhengwei143 November 10, 2023 16:31

zhengwei143 requested changes Nov 16, 2023

View reviewed changes

Feedback

52c72c6

AlessandroPatti requested a review from zhengwei143 November 20, 2023 09:31

zhengwei143 approved these changes Nov 21, 2023

View reviewed changes

copybara-service bot closed this in ebe4d0d Nov 28, 2023

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Nov 28, 2023

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 28, 2023

bazel-io mentioned this pull request Nov 28, 2023

[7.0.0] Generalize memory and cpu tracking #20344

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 28, 2023

iancha1992 mentioned this pull request Nov 30, 2023

[7.0.0] Generalize memory and cpu tracking #20396

Merged

AlessandroPatti mentioned this pull request Nov 30, 2023

Introduce --local_resources flag #20398

Closed

bazel-io mentioned this pull request Feb 13, 2024

[7.1.0] Introduce --local_resources flag #21331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize memory and cpu tracking #19839

Generalize memory and cpu tracking #19839

AlessandroPatti commented Oct 17, 2023

zhengwei143 commented Nov 13, 2023

zhengwei143 Nov 16, 2023

AlessandroPatti Nov 18, 2023

zhengwei143 Nov 16, 2023

AlessandroPatti Nov 18, 2023

zhengwei143 Nov 16, 2023

AlessandroPatti Nov 18, 2023

zhengwei143 Nov 21, 2023

zhengwei143 Nov 16, 2023

AlessandroPatti Nov 18, 2023

zhengwei143 Nov 16, 2023

AlessandroPatti Nov 18, 2023

zhengwei143 left a comment

fmeum commented Nov 21, 2023

zhengwei143 commented Nov 22, 2023

brentleyjones commented Nov 28, 2023

keertk commented Nov 28, 2023

fmeum commented Nov 28, 2023

keertk commented Nov 28, 2023

AlessandroPatti commented Nov 30, 2023

keertk commented Nov 30, 2023

iancha1992 commented Dec 1, 2023

		@@ -37,19 +37,13 @@
		public class ResourceSet implements ResourceSetOrBuilder {

Generalize memory and cpu tracking #19839

Generalize memory and cpu tracking #19839

Conversation

AlessandroPatti commented Oct 17, 2023

zhengwei143 commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengwei143 left a comment

Choose a reason for hiding this comment

fmeum commented Nov 21, 2023

zhengwei143 commented Nov 22, 2023

brentleyjones commented Nov 28, 2023

keertk commented Nov 28, 2023

fmeum commented Nov 28, 2023

keertk commented Nov 28, 2023

AlessandroPatti commented Nov 30, 2023

keertk commented Nov 30, 2023

iancha1992 commented Dec 1, 2023