From d06dbc40a6ae2044626d109bf8a44e8baf06a887 Mon Sep 17 00:00:00 2001 From: Alessandro Patti Date: Tue, 9 Jan 2024 05:46:39 -0800 Subject: [PATCH] Introduce --local_resources flag Follow up for https://github.com/bazelbuild/bazel/pull/19839#discussion_r1400304039, introduce new flag that will replace all the others. Closes #20398. PiperOrigin-RevId: 596905096 Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52 --- .../build/lib/analysis/AnalysisOptions.java | 20 +- .../google/devtools/build/lib/analysis/BUILD | 1 + .../lib/buildtool/BuildRequestOptions.java | 24 +-- .../build/lib/buildtool/ExecutionTool.java | 9 +- .../build/lib/exec/ExecutionOptions.java | 68 +++++-- .../IncludeScanningOptions.java | 9 +- .../build/lib/pkgcache/PackageOptions.java | 9 +- .../runtime/LoadingPhaseThreadsOption.java | 8 +- .../build/lib/sandbox/SandboxOptions.java | 8 +- .../com/google/devtools/build/lib/util/BUILD | 3 +- .../build/lib/util/CpuResourceConverter.java | 14 +- .../build/lib/util/RamResourceConverter.java | 11 +- .../build/lib/util/ResourceConverter.java | 190 ++++++++++++------ .../build/lib/worker/WorkerOptions.java | 4 +- .../build/lib/util/ResourceConverterTest.java | 43 ++-- .../worker/MultiResourceConverterTest.java | 4 +- .../remote/worker/RemoteWorkerOptions.java | 10 +- 17 files changed, 262 insertions(+), 173 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java index d00002a4831ca4..a915fed5b8e46b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java @@ -16,6 +16,7 @@ import com.google.devtools.build.lib.util.CpuResourceConverter; import com.google.devtools.build.lib.util.RegexFilter; +import com.google.devtools.build.lib.util.ResourceConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -106,7 +107,7 @@ public class AnalysisOptions extends OptionsBase { @Option( name = "experimental_skyframe_cpu_heavy_skykeys_thread_pool_size", - defaultValue = "HOST_CPUS", + defaultValue = ResourceConverter.HOST_CPUS_KEYWORD, documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, metadataTags = OptionMetadataTag.EXPERIMENTAL, effectTags = { @@ -114,16 +115,21 @@ public class AnalysisOptions extends OptionsBase { OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION }, help = - "If set to a positive value (e.g. \"HOST_CPUS*1.5\"), Skyframe will run the" - + " loading/analysis phase with 2 separate thread pools: 1 with threads" - + " (ideally close to HOST_CPUS) reserved for CPU-heavy SkyKeys, and 1 \"standard\"" + "If set to a positive value (e.g. \"" + + ResourceConverter.HOST_CPUS_KEYWORD + + "*1.5\")," + + " Skyframe will run the loading/analysis phase with 2 separate thread pools:" + + " 1 with threads (ideally close to " + + ResourceConverter.HOST_CPUS_KEYWORD + + ")" + + " reserved for CPU-heavy SkyKeys, and 1 \"standard\"" + " thread pool (whose size is controlled by --loading_phase_threads) for the rest.", converter = CpuResourceConverter.class) public int cpuHeavySkyKeysThreadPoolSize; @Option( name = "experimental_oom_sensitive_skyfunctions_semaphore_size", - defaultValue = "HOST_CPUS", + defaultValue = ResourceConverter.HOST_CPUS_KEYWORD, documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, metadataTags = OptionMetadataTag.EXPERIMENTAL, effectTags = { @@ -133,7 +139,9 @@ public class AnalysisOptions extends OptionsBase { help = "Sets the size of the semaphore used to prevent SkyFunctions with large peak memory" + " requirement from OOM-ing blaze. A value of 0 indicates that no semaphore should" - + " be used. Example value: \"HOST_CPUS*0.5\".", + + " be used. Example value: \"" + + ResourceConverter.HOST_CPUS_KEYWORD + + "*0.5\".", converter = CpuResourceConverter.class) public int oomSensitiveSkyFunctionsSemaphoreSize; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index d4b3274e297809..189a962464aa20 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -473,6 +473,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:cpu_resource_converter", + "//src/main/java/com/google/devtools/build/lib/util:resource_converter", "//src/main/java/com/google/devtools/common/options", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index 86b8f4569055d7..7f2b93f4388724 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -16,7 +16,6 @@ import com.github.benmanes.caffeine.cache.CaffeineSpec; import com.google.common.annotations.VisibleForTesting; import com.google.common.flogger.GoogleLogger; -import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.build.lib.util.OptionsUtils; import com.google.devtools.build.lib.util.ResourceConverter; import com.google.devtools.build.lib.vfs.PathFragment; @@ -399,13 +398,9 @@ public String getSymlinkPrefix(String productName) { public int skymeldAnalysisOverlapPercentage; /** Converter for filesystem value checker threads. */ - public static class ThreadConverter extends ResourceConverter { + public static class ThreadConverter extends ResourceConverter.IntegerConverter { public ThreadConverter() { - super( - /* autoSupplier= */ () -> - (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()), - /* minValue= */ 1, - /* maxValue= */ Integer.MAX_VALUE); + super(/* auto= */ HOST_CPUS_SUPPLIER, /* minValue= */ 1, /* maxValue= */ Integer.MAX_VALUE); } } @@ -448,27 +443,24 @@ public ThreadConverter() { * Converter for jobs: Takes keyword ({@value #FLAG_SYNTAX}). Values must be between 1 and * MAX_JOBS. */ - public static class JobsConverter extends ResourceConverter { + public static class JobsConverter extends ResourceConverter.IntegerConverter { public JobsConverter() { - super( - () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()), - 1, - MAX_JOBS); + super(/* auto= */ HOST_CPUS_SUPPLIER, /* minValue= */ 1, /* maxValue= */ MAX_JOBS); } @Override - public int checkAndLimit(int value) throws OptionsParsingException { - if (value < minValue) { + public Integer checkAndLimit(Integer value) throws OptionsParsingException { + if (value.doubleValue() < minValue) { throw new OptionsParsingException( String.format("Value '(%d)' must be at least %d.", value, minValue)); } - if (value > maxValue) { + if (value.doubleValue() > maxValue) { logger.atWarning().log( "Flag remoteWorker \"jobs\" ('%d') was set too high. " + "This is a result of passing large values to --local_resources or --jobs. " + "Using '%d' jobs", value, maxValue); - value = maxValue; + return maxValue; } return value; } diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index fccf74b29ae274..ec4ce8460fa4b0 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; +import com.google.common.collect.Streams; import com.google.common.eventbus.Subscribe; import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.Action; @@ -115,7 +116,6 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.stream.Stream; import javax.annotation.Nullable; /** @@ -996,11 +996,16 @@ public static void configureResourceManager(ResourceManager resourceMgr, BuildRe ImmutableMap cpuRam = ImmutableMap.of( ResourceSet.CPU, + // Replace with 1.0 * ResourceConverter.HOST_CPUS.get() after flag deprecation options.localCpuResources, ResourceSet.MEMORY, + // Replace with 0.67 * ResourceConverter.HOST_RAM.get() after flag deprecation options.localRamResources); ImmutableMap resources = - Stream.concat(options.localExtraResources.stream(), cpuRam.entrySet().stream()) + Streams.concat( + options.localExtraResources.stream(), + cpuRam.entrySet().stream(), + options.localResources.stream()) .collect( ImmutableMap.toImmutableMap( Map.Entry::getKey, Map.Entry::getValue, (v1, v2) -> v2)); diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java index 9806ac021b7c7a..782fbe2f13c149 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java @@ -293,35 +293,54 @@ public boolean shouldMaterializeParamFiles() { @Option( name = "local_cpu_resources", - defaultValue = "HOST_CPUS", + defaultValue = ResourceConverter.HOST_CPUS_KEYWORD, + deprecationWarning = + "--local_cpu_resources is deprecated, please use --local_resources=cpu= instead.", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = "Explicitly set the total number of local CPU cores available to Bazel to spend on build" - + " actions executed locally. Takes an integer, or \"HOST_CPUS\", optionally followed" - + " by [-|*] (eg. HOST_CPUS*.5 to use half the available CPU cores).By" - + " default, (\"HOST_CPUS\"), Bazel will query system configuration to estimate" - + " the number of CPU cores available.", + + " actions executed locally. Takes an integer, or \"" + + ResourceConverter.HOST_CPUS_KEYWORD + + "\", optionally followed" + + " by [-|*] (eg. " + + ResourceConverter.HOST_CPUS_KEYWORD + + "*.5" + + " to use half the available CPU cores). By default, (\"" + + ResourceConverter.HOST_CPUS_KEYWORD + + "\"), Bazel will query system" + + " configuration to estimate the number of CPU cores available.", converter = CpuResourceConverter.class) public double localCpuResources; @Option( name = "local_ram_resources", - defaultValue = "HOST_RAM*.67", + defaultValue = ResourceConverter.HOST_RAM_KEYWORD + "*.67", + deprecationWarning = + "--local_ram_resources is deprecated, please use --local_resources=memory= instead.", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = "Explicitly set the total amount of local host RAM (in MB) available to Bazel to spend on" - + " build actions executed locally. Takes an integer, or \"HOST_RAM\", optionally" - + " followed by [-|*] (eg. HOST_RAM*.5 to use half the available RAM). By" - + " default, (\"HOST_RAM*.67\"), Bazel will query system configuration to estimate" - + " the amount of RAM available and will use 67% of it.", + + " build actions executed locally. Takes an integer, or \"" + + ResourceConverter.HOST_RAM_KEYWORD + + "\", optionally followed by [-|*]" + + " (eg. " + + ResourceConverter.HOST_RAM_KEYWORD + + "*.5 to use half the available" + + " RAM). By default, (\"" + + ResourceConverter.HOST_RAM_KEYWORD + + "*.67\")," + + " Bazel will query system configuration to estimate the amount of RAM available" + + " and will use 67% of it.", converter = RamResourceConverter.class) public double localRamResources; @Option( name = "local_extra_resources", defaultValue = "null", + deprecationWarning = + "--local_extra_resources is deprecated, please use --local_resources instead.", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, allowMultiple = true, @@ -336,6 +355,31 @@ public boolean shouldMaterializeParamFiles() { converter = Converters.StringToDoubleAssignmentConverter.class) public List> localExtraResources; + @Option( + name = "local_resources", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, + effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, + allowMultiple = true, + help = + "Set the number of resources available to Bazel. " + + "Takes in an assignment to a float or " + + ResourceConverter.HOST_RAM_KEYWORD + + "/" + + ResourceConverter.HOST_CPUS_KEYWORD + + ", optionally " + + "followed by [-|*] (eg. memory=" + + ResourceConverter.HOST_RAM_KEYWORD + + "*.5 to use half the available RAM). " + + "Can be used multiple times to specify multiple " + + "types of resources. Bazel will limit concurrently running actions " + + "based on the available resources and the resources required. " + + "Tests can declare the amount of resources they need " + + "by using a tag of the \"resources::\" format. " + + "Overrides resources specified by --local_{cpu|ram|extra}_resources.", + converter = ResourceConverter.AssignmentConverter.class) + public List> localResources; + @Option( name = "local_test_jobs", defaultValue = "auto", @@ -574,9 +618,9 @@ public String getTypeDescription() { } /** Converter for --local_test_jobs, which takes {@value FLAG_SYNTAX} */ - public static class LocalTestJobsConverter extends ResourceConverter { + public static class LocalTestJobsConverter extends ResourceConverter.IntegerConverter { public LocalTestJobsConverter() throws OptionsParsingException { - super(/* autoSupplier= */ () -> 0, /* minValue= */ 0, /* maxValue= */ Integer.MAX_VALUE); + super(/* auto= */ () -> 0, /* minValue= */ 0, /* maxValue= */ Integer.MAX_VALUE); } } diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java index 156a03c2849ed8..b6a056a877873a 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningOptions.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.includescanning; -import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.build.lib.util.ResourceConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; @@ -30,13 +29,9 @@ public class IncludeScanningOptions extends OptionsBase { * Converter for scanning parallelism threads: Takes {@value #FLAG_SYNTAX} 0 disables scanning * parallelism. */ - public static class ParallelismConverter extends ResourceConverter { + public static class ParallelismConverter extends ResourceConverter.IntegerConverter { public ParallelismConverter() throws OptionsParsingException { - super( - /* autoSupplier= */ () -> - (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()), - /* minValue= */ 0, - /* maxValue= */ Integer.MAX_VALUE); + super(/* auto= */ HOST_CPUS_SUPPLIER, /* minValue= */ 0, /* maxValue= */ Integer.MAX_VALUE); } } diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java index 278710464c09df..b69e2001bf15c5 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java @@ -19,7 +19,6 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.packages.RuleVisibility; @@ -59,13 +58,9 @@ public String getTypeDescription() { } /** Converter for globbing threads. */ - public static class ParallelismConverter extends ResourceConverter { + public static class ParallelismConverter extends ResourceConverter.IntegerConverter { public ParallelismConverter() throws OptionsParsingException { - super( - /* autoSupplier= */ () -> - (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()), - /* minValue= */ 1, - /* maxValue= */ Integer.MAX_VALUE); + super(/* auto= */ HOST_CPUS_SUPPLIER, /* minValue= */ 1, /* maxValue= */ Integer.MAX_VALUE); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/LoadingPhaseThreadsOption.java b/src/main/java/com/google/devtools/build/lib/runtime/LoadingPhaseThreadsOption.java index 7340232ed958e4..32c18829abbcab 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/LoadingPhaseThreadsOption.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/LoadingPhaseThreadsOption.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.runtime; import com.google.common.flogger.GoogleLogger; -import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.build.lib.util.ResourceConverter; import com.google.devtools.build.lib.util.TestType; import com.google.devtools.common.options.Option; @@ -45,18 +44,19 @@ public class LoadingPhaseThreadsOption extends OptionsBase { /** * A converter for loading phase thread count. Takes {@value FLAG_SYNTAX}. Caps at 20 for tests. */ - public static final class LoadingPhaseThreadCountConverter extends ResourceConverter { + public static final class LoadingPhaseThreadCountConverter + extends ResourceConverter.IntegerConverter { public LoadingPhaseThreadCountConverter() { // TODO(jmmv): Using the number of cores has proven to yield reasonable analysis times on // Mac Pros and MacBook Pros but we should probably do better than this. (We haven't made // any guarantees that "auto" means number of cores precisely to leave us room to tune this // further in the future.) - super(() -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage())); + super(/* auto= */ HOST_CPUS_SUPPLIER, /* minValue= */ 1, /* maxValue= */ Integer.MAX_VALUE); } @Override - public int checkAndLimit(int value) throws OptionsParsingException { + public Integer checkAndLimit(Integer value) throws OptionsParsingException { // Cap thread count while running tests. Test cases are typically small and large thread // pools vying for a relatively small number of CPU cores may induce non-optimal // performance. diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java index 8833e8615d53a3..64b6ca087abcee 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java @@ -18,7 +18,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.build.lib.util.OptionsUtils; import com.google.devtools.build.lib.util.RamResourceConverter; import com.google.devtools.build.lib.util.ResourceConverter; @@ -371,12 +370,9 @@ public ImmutableSet getInaccessiblePaths(FileSystem fs) { public int memoryLimitMb; /** Converter for the number of threads used for asynchronous tree deletion. */ - public static final class AsyncTreeDeletesConverter extends ResourceConverter { + public static final class AsyncTreeDeletesConverter extends ResourceConverter.IntegerConverter { public AsyncTreeDeletesConverter() { - super( - () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()), - 0, - Integer.MAX_VALUE); + super(/* auto= */ HOST_CPUS_SUPPLIER, /* minValue= */ 0, /* maxValue= */ Integer.MAX_VALUE); } } } diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD index a7519e1fe9adf7..e7c57f38b53239 100644 --- a/src/main/java/com/google/devtools/build/lib/util/BUILD +++ b/src/main/java/com/google/devtools/build/lib/util/BUILD @@ -127,6 +127,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/common/options", + "//third_party:error_prone_annotations", "//third_party:guava", "//third_party:jsr305", ], @@ -139,7 +140,6 @@ java_library( ], deps = [ ":resource_converter", - "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//third_party:guava", ], ) @@ -151,7 +151,6 @@ java_library( ], deps = [ ":resource_converter", - "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/util/CpuResourceConverter.java b/src/main/java/com/google/devtools/build/lib/util/CpuResourceConverter.java index aea425252965f7..d6fe85b0244753 100644 --- a/src/main/java/com/google/devtools/build/lib/util/CpuResourceConverter.java +++ b/src/main/java/com/google/devtools/build/lib/util/CpuResourceConverter.java @@ -14,24 +14,22 @@ package com.google.devtools.build.lib.util; import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.actions.LocalHostCapacity; /** * Converter for --local_cpu_resources, which takes an integer greater than or equal to 1, or * "HOST_CPUS", optionally followed by [-|*]. */ -public final class CpuResourceConverter extends ResourceConverter { +public final class CpuResourceConverter extends ResourceConverter.IntegerConverter { public CpuResourceConverter() { super( - ImmutableMap.of( - "HOST_CPUS", - () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage())), - /*minValue=*/ 0, - Integer.MAX_VALUE); + /* keywords= */ ImmutableMap.of(HOST_CPUS_KEYWORD, HOST_CPUS_SUPPLIER), + /* minValue= */ 0, + /* maxValue= */ Integer.MAX_VALUE); } @Override public String getTypeDescription() { - return "an integer, or \"HOST_CPUS\", optionally followed by [-|*]."; + return String.format( + "an integer, or \"%s\", optionally followed by [-|*].", HOST_CPUS_KEYWORD); } } diff --git a/src/main/java/com/google/devtools/build/lib/util/RamResourceConverter.java b/src/main/java/com/google/devtools/build/lib/util/RamResourceConverter.java index 647fcf653ed76c..129be25ed62c2c 100644 --- a/src/main/java/com/google/devtools/build/lib/util/RamResourceConverter.java +++ b/src/main/java/com/google/devtools/build/lib/util/RamResourceConverter.java @@ -14,24 +14,23 @@ package com.google.devtools.build.lib.util; import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.actions.LocalHostCapacity; /** * Converter for --local_cpu_resources, which takes an integer greater than or equal to 0, or * "HOST_RAM", optionally followed by [-|*]. */ -public final class RamResourceConverter extends ResourceConverter { +public final class RamResourceConverter extends ResourceConverter.IntegerConverter { public RamResourceConverter() { super( - /* keywords= */ ImmutableMap.of( - "HOST_RAM", - () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getMemoryMb())), + /* keywords= */ ImmutableMap.of(HOST_RAM_KEYWORD, HOST_RAM_SUPPLIER), /* minValue= */ 0, /* maxValue= */ Integer.MAX_VALUE); } @Override public String getTypeDescription() { - return "an integer number of MBs, or \"HOST_RAM\", optionally followed by [-|*]."; + return String.format( + "an integer number of MBs, or \"%s\", optionally followed by [-|*].", + HOST_RAM_KEYWORD); } } diff --git a/src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java b/src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java index 5f666b893f300e..f50d19d4ed3190 100644 --- a/src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java +++ b/src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java @@ -15,10 +15,13 @@ package com.google.devtools.build.lib.util; import com.google.common.collect.ImmutableMap; +import com.google.common.primitives.Doubles; import com.google.common.primitives.Ints; import com.google.devtools.build.lib.actions.LocalHostCapacity; +import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.OptionsParsingException; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.Map; import java.util.function.DoubleBinaryOperator; import java.util.function.Supplier; @@ -39,7 +42,97 @@ *

The supplier of the auto value, and, optionally, a max or min allowed value (inclusive), are * passed to the constructor. */ -public class ResourceConverter extends Converters.IntegerConverter { +public abstract class ResourceConverter> + extends Converter.Contextless { + public static final String AUTO_KEYWORD = "auto"; + public static final String HOST_CPUS_KEYWORD = "HOST_CPUS"; + public static final String HOST_RAM_KEYWORD = "HOST_RAM"; + + public static final Supplier HOST_CPUS_SUPPLIER = + () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()); + public static final Supplier HOST_RAM_SUPPLIER = + () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getMemoryMb()); + + /** Resource converter for assignments. */ + public static class AssignmentConverter extends Converter.Contextless> { + private static final Converters.AssignmentConverter assignment = + new Converters.AssignmentConverter(); + private static final ResourceConverter.DoubleConverter resource = + new ResourceConverter.DoubleConverter( + ImmutableMap.of( + HOST_CPUS_KEYWORD, () -> (double) HOST_CPUS_SUPPLIER.get(), + HOST_RAM_KEYWORD, () -> (double) HOST_RAM_SUPPLIER.get()), + 0.0, + Double.MAX_VALUE); + + @Override + public Map.Entry convert(String input) throws OptionsParsingException { + Map.Entry s = assignment.convert(input); + return Map.entry(s.getKey(), resource.convert(s.getValue())); + } + + @Override + public String getTypeDescription() { + return "a named double, 'name=value', where value is " + resource.getTypeDescription(); + } + } + + /** Resource converter for integers. */ + public static class IntegerConverter extends ResourceConverter { + private static final Converters.IntegerConverter converter = new Converters.IntegerConverter(); + + public IntegerConverter(Supplier auto, int minValue, int maxValue) { + this( + ImmutableMap.of( + AUTO_KEYWORD, + auto, + HOST_CPUS_KEYWORD, + HOST_CPUS_SUPPLIER, + HOST_RAM_KEYWORD, + HOST_RAM_SUPPLIER), + minValue, + maxValue); + } + + public IntegerConverter( + ImmutableMap> keywords, int minValue, int maxValue) { + super(keywords, minValue, maxValue); + } + + @Override + public Integer convert(String input) throws OptionsParsingException { + return Ints.tryParse(input) != null + ? checkAndLimit(converter.convert(input)) + : checkAndLimit((int) Math.round(convertKeyword(input))); + } + } + + /** Resource converter for doubles. */ + public static class DoubleConverter extends ResourceConverter { + private static final Converters.DoubleConverter converter = new Converters.DoubleConverter(); + + public DoubleConverter(Supplier auto, double minValue, double maxValue) { + this( + ImmutableMap.of( + AUTO_KEYWORD, auto, + HOST_CPUS_KEYWORD, () -> (double) HOST_CPUS_SUPPLIER.get(), + HOST_RAM_KEYWORD, () -> (double) HOST_RAM_SUPPLIER.get()), + minValue, + maxValue); + } + + public DoubleConverter( + ImmutableMap> keywords, double minValue, double maxValue) { + super(keywords, minValue, maxValue); + } + + @Override + public Double convert(String input) throws OptionsParsingException { + return Doubles.tryParse(input) != null + ? checkAndLimit(converter.convert(input)) + : convertKeyword(input); + } + } private static final ImmutableMap OPERATORS = ImmutableMap.builder() @@ -49,56 +142,32 @@ public class ResourceConverter extends Converters.IntegerConverter { /** Description of the accepted inputs to the converter. */ public static final String FLAG_SYNTAX = - "an integer, or a keyword (\"auto\", \"HOST_CPUS\", \"HOST_RAM\"), optionally followed by " - + "an operation ([-|*]) eg. \"auto\", \"HOST_CPUS*.5\""; - - private final ImmutableMap> keywords; + "an integer, or a keyword (\"" + + AUTO_KEYWORD + + "\", \"" + + HOST_CPUS_KEYWORD + + "\", \"" + + HOST_RAM_KEYWORD + + "\"), optionally followed by an operation ([-|*]) eg. \"" + + AUTO_KEYWORD + + "\", \"" + + HOST_CPUS_KEYWORD + + "*.5\""; + + private final ImmutableMap> keywords; private final Pattern validInputPattern; - protected final int minValue; - - protected final int maxValue; + protected final T minValue; - /** - * Constructs a ResourceConverter for options that take {@value FLAG_SYNTAX} - * - * @param autoSupplier a supplier for the value of the auto keyword - * @param minValue the minimum allowed value - * @param maxValue the maximum allowed value - */ - public ResourceConverter(Supplier autoSupplier, int minValue, int maxValue) { - this( - ImmutableMap.>builder() - .put("auto", autoSupplier) - .put( - "HOST_CPUS", - () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage())) - .put( - "HOST_RAM", - () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getMemoryMb())) - .build(), - minValue, - maxValue); - } - - /** - * Constructs a ResourceConverter for options that take {@value FLAG_SYNTAX} and accept any value - * greater than 1. - * - * @param autoSupplier a supplier for the value of the auto keyword - */ - public ResourceConverter(Supplier autoSupplier) { - this(autoSupplier, 1, Integer.MAX_VALUE); - } + protected final T maxValue; /** * Constructs a ResourceConverter for options that take keywords other than the default set. * * @param keywords a map of keyword to the suppliers of their values */ - public ResourceConverter( - ImmutableMap> keywords, int minValue, int maxValue) { + public ResourceConverter(ImmutableMap> keywords, T minValue, T maxValue) { this.keywords = keywords; this.validInputPattern = Pattern.compile( @@ -109,19 +178,12 @@ public ResourceConverter( this.maxValue = maxValue; } - @Override - public final Integer convert(String input) throws OptionsParsingException { - int value; - if (Ints.tryParse(input) != null) { - value = super.convert(input); - return checkAndLimit(value); - } + public final Double convertKeyword(String input) throws OptionsParsingException { Matcher matcher = validInputPattern.matcher(input); if (matcher.matches()) { - Supplier resourceSupplier = keywords.get(matcher.group("keyword")); + Supplier resourceSupplier = keywords.get(matcher.group("keyword")); if (resourceSupplier != null) { - value = applyOperator(matcher.group("expression"), resourceSupplier); - return checkAndLimit(value); + return applyOperator(matcher.group("expression"), resourceSupplier); } } throw new OptionsParsingException( @@ -131,10 +193,10 @@ public final Integer convert(String input) throws OptionsParsingException { } /** Applies function designated in {@code expression} ([-|*]) to value. */ - private Integer applyOperator(@Nullable String expression, Supplier firstOperandSupplier) + private Double applyOperator(@Nullable String expression, Supplier firstOperandSupplier) throws OptionsParsingException { if (expression == null) { - return firstOperandSupplier.get(); + return firstOperandSupplier.get().doubleValue(); } for (Map.Entry operator : OPERATORS.entrySet()) { if (expression.startsWith(operator.getKey())) { @@ -146,11 +208,9 @@ private Integer applyOperator(@Nullable String expression, Supplier fir String.format("'%s is not a float", expression.substring(operator.getKey().length())), e); } - return (int) - Math.round( - operator - .getValue() - .applyAsDouble((float) firstOperandSupplier.get(), secondOperand)); + return operator + .getValue() + .applyAsDouble(firstOperandSupplier.get().doubleValue(), secondOperand); } } // This should never happen because we've checked for a valid operator already. @@ -162,14 +222,18 @@ private Integer applyOperator(@Nullable String expression, Supplier fir * Checks validity of a resource value against min/max constraints. Implementations may choose to * either raise an exception on out-of-bounds values, or adjust them to within the constraints. */ - public int checkAndLimit(int value) throws OptionsParsingException { - if (value < minValue) { + @CanIgnoreReturnValue + public T checkAndLimit(T value) throws OptionsParsingException { + if (value.compareTo(minValue) < 0) { throw new OptionsParsingException( - String.format("Value '(%d)' must be at least %d.", value, minValue)); + String.format( + "Value '(%f)' must be at least %f.", value.doubleValue(), minValue.doubleValue())); } - if (value > maxValue) { + if (value.compareTo(maxValue) > 0) { throw new OptionsParsingException( - String.format("Value '(%d)' cannot be greater than %d.", value, maxValue)); + String.format( + "Value '(%f)' cannot be greater than %f.", + value.doubleValue(), maxValue.doubleValue())); } return value; } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java index 35158766aece1d..271f2dbe5dd209 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java @@ -44,8 +44,8 @@ public class WorkerOptions extends OptionsBase { */ public static class MultiResourceConverter extends Converter.Contextless> { - static final ResourceConverter valueConverter = - new ResourceConverter(() -> 0, 0, Integer.MAX_VALUE); + static final ResourceConverter.IntegerConverter valueConverter = + new ResourceConverter.IntegerConverter(() -> 0, 0, Integer.MAX_VALUE); @Override public Map.Entry convert(String input) throws OptionsParsingException { diff --git a/src/test/java/com/google/devtools/build/lib/util/ResourceConverterTest.java b/src/test/java/com/google/devtools/build/lib/util/ResourceConverterTest.java index fa1d8e7f686363..b5e8141b6ff7a0 100644 --- a/src/test/java/com/google/devtools/build/lib/util/ResourceConverterTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/ResourceConverterTest.java @@ -28,17 +28,23 @@ @RunWith(JUnit4.class) public class ResourceConverterTest { - private ResourceConverter resourceConverter; + private ResourceConverter resourceConverter; @Test public void convertNumber_returnsInt() throws Exception { - resourceConverter = new ResourceConverter(() -> null); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 1, Integer.MAX_VALUE); assertThat(resourceConverter.convert("6")).isEqualTo(6); } + @Test + public void convertNumber_returnsDouble() throws Exception { + resourceConverter = new ResourceConverter.DoubleConverter(() -> null, 1.0, Double.MAX_VALUE); + assertThat(resourceConverter.convert("6.3")).isEqualTo(6.3); + } + @Test public void convertNumber_greaterThanMax_throwsException() { - resourceConverter = new ResourceConverter(() -> null, 0, 1); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 0, 1); OptionsParsingException thrown = assertThrows(OptionsParsingException.class, () -> resourceConverter.convert("2")); assertThat(thrown).hasMessageThat().contains("cannot be greater than 1"); @@ -46,7 +52,7 @@ public void convertNumber_greaterThanMax_throwsException() { @Test public void convertNumber_lessThanMin_throwsException() throws Exception { - resourceConverter = new ResourceConverter(() -> null, -1, 1); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, -1, 1); assertThat(resourceConverter.convert("0")).isEqualTo(0); OptionsParsingException thrown = assertThrows(OptionsParsingException.class, () -> resourceConverter.convert("-2")); @@ -55,19 +61,19 @@ public void convertNumber_lessThanMin_throwsException() throws Exception { @Test public void convertAuto_returnsSuppliedAutoValue() throws Exception { - resourceConverter = new ResourceConverter(() -> 5); + resourceConverter = new ResourceConverter.IntegerConverter(() -> 5, 1, Integer.MAX_VALUE); assertThat(resourceConverter.convert("auto")).isEqualTo(5); } @Test public void convertAuto_withOperator_appliesOperatorToAuto() throws Exception { - resourceConverter = new ResourceConverter(() -> 5); + resourceConverter = new ResourceConverter.IntegerConverter(() -> 5, 1, Integer.MAX_VALUE); assertThat(resourceConverter.convert("auto-1")).isEqualTo(4); } @Test public void convertAuto_withInvalidOperator_throwsException() { - resourceConverter = new ResourceConverter(() -> null); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 1, Integer.MAX_VALUE); OptionsParsingException thrown = assertThrows(OptionsParsingException.class, () -> resourceConverter.convert("auto/2")); assertThat(thrown).hasMessageThat().contains("does not follow correct syntax"); @@ -75,27 +81,27 @@ public void convertAuto_withInvalidOperator_throwsException() { @Test public void convertAuto_isFloat_returnsRoundedInt() throws Exception { - resourceConverter = new ResourceConverter(() -> 5); + resourceConverter = new ResourceConverter.IntegerConverter(() -> 5, 1, Integer.MAX_VALUE); assertThat(resourceConverter.convert("auto*.51")).isEqualTo(3); } @Test public void convertHostCpus_returnsCpuSetting() throws Exception { LocalHostCapacity.setLocalHostCapacity(ResourceSet.createWithRamCpu(1, 15)); - resourceConverter = new ResourceConverter(() -> 5); + resourceConverter = new ResourceConverter.IntegerConverter(() -> 5, 1, Integer.MAX_VALUE); assertThat(resourceConverter.convert("HOST_CPUS")).isEqualTo(15); } @Test public void convertRam_returnsRamSetting() throws Exception { LocalHostCapacity.setLocalHostCapacity(ResourceSet.createWithRamCpu(10, 0)); - resourceConverter = new ResourceConverter(() -> 5); + resourceConverter = new ResourceConverter.IntegerConverter(() -> 5, 1, Integer.MAX_VALUE); assertThat(resourceConverter.convert("HOST_RAM")).isEqualTo(10); } @Test public void convertFloat_throwsException() { - resourceConverter = new ResourceConverter(() -> null); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 1, Integer.MAX_VALUE); OptionsParsingException thrown = assertThrows(OptionsParsingException.class, () -> resourceConverter.convert(".5")); assertThat(thrown).hasMessageThat().contains("This flag takes an integer"); @@ -103,7 +109,7 @@ public void convertFloat_throwsException() { @Test public void convertWrongKeyword_throwsException() { - resourceConverter = new ResourceConverter(() -> null); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 1, Integer.MAX_VALUE); OptionsParsingException thrown = assertThrows( OptionsParsingException.class, () -> resourceConverter.convert("invalid_keyword")); @@ -119,7 +125,7 @@ public void convertWrongKeyword_throwsException() { @Test public void convertAlmostValidKeyword_throwsException() { - resourceConverter = new ResourceConverter(() -> null); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 1, Integer.MAX_VALUE); OptionsParsingException thrown = assertThrows(OptionsParsingException.class, () -> resourceConverter.convert("aut")); assertThat(thrown).hasMessageThat().contains("does not follow correct syntax"); @@ -127,17 +133,8 @@ public void convertAlmostValidKeyword_throwsException() { @Test public void buildConverter_beforeResources_usesResources() throws Exception { - resourceConverter = new ResourceConverter(() -> null); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 1, Integer.MAX_VALUE); LocalHostCapacity.setLocalHostCapacity(ResourceSet.createWithRamCpu(0, 15)); assertThat(resourceConverter.convert("HOST_CPUS")).isEqualTo(15); } - - @Test - public void buildConverter_withNoMin_setsMinTo1() { - resourceConverter = new ResourceConverter(() -> null); - OptionsParsingException thrown = - assertThrows(OptionsParsingException.class, () -> resourceConverter.convert("0")); - assertThat(thrown).hasMessageThat().contains("must be at least 1"); - } - } diff --git a/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java b/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java index d5deb2bafedc47..34034531626f97 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java @@ -28,12 +28,12 @@ public class MultiResourceConverterTest { public MultiResourceConverter multiResourceConverter; - public ResourceConverter resourceConverter; + public ResourceConverter resourceConverter; @Before public void setUp() { multiResourceConverter = new MultiResourceConverter(); - resourceConverter = new ResourceConverter(() -> null); + resourceConverter = new ResourceConverter.IntegerConverter(() -> null, 1, Integer.MAX_VALUE); } @Test diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java index 5a228c8bc3620a..493caea3a670bd 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorkerOptions.java @@ -15,7 +15,6 @@ package com.google.devtools.build.remote.worker; import com.google.common.flogger.GoogleLogger; -import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.build.lib.util.ResourceConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; @@ -192,16 +191,13 @@ public class RemoteWorkerOptions extends OptionsBase { * Converter for jobs. Takes {@value FLAG_SYNTAX}. Values must be between 1 and {@value MAX_JOBS}. * Values higher than {@value MAX_JOBS} will be set to {@value MAX_JOBS}. */ - public static class JobsConverter extends ResourceConverter { + public static class JobsConverter extends ResourceConverter.IntegerConverter { public JobsConverter() { - super( - () -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()), - 1, - MAX_JOBS); + super(/* auto= */ HOST_CPUS_SUPPLIER, /* minValue= */ 1, /* maxValue= */ MAX_JOBS); } @Override - public int checkAndLimit(int value) throws OptionsParsingException { + public Integer checkAndLimit(Integer value) throws OptionsParsingException { if (value < minValue) { throw new OptionsParsingException( String.format("Value '(%d)' must be at least %d.", value, minValue));