From ac880418885061d1039ad6b3d8c28949782e02d6 Mon Sep 17 00:00:00 2001 From: jmmv Date: Sun, 30 Sep 2018 19:08:22 -0700 Subject: [PATCH] Make --loading_phase_threads default to the number of CPUs. Change the behavior of the --loading_phase_threads to match that of --jobs: default it to a new "auto" magic value that causes Bazel to determine a reasonable setting (currently the number of CPUs) but allow users to override it with an explicit count. This should alleviate threading contention during analysis and reduce the unresponsiveness of macOS when this happens. Some tests on large apps show a reduction of up to 40% (!) on a MacBook Pro with 4 physical cores and a more modest 15% on a Mac Pro with 6 physical cores. Note that the previous hack to cap the number of threads to 20 for unit and integration tests remains. I'll tackle this separately. RELNOTES: --loading_phase_threads now defaults to "auto" (not 200, as was previously the case), which at the moment corresponds to the number of CPUs. If your sources are on a slow file system, increasing this value may yield better analysis-time performance when disk caches are cold. PiperOrigin-RevId: 215151994 --- .../java/com/google/devtools/build/lib/BUILD | 5 +- .../runtime/LoadingPhaseThreadsOption.java | 56 ++++++++++++------- .../integration/execution_phase_tests.sh | 25 +++++++-- 3 files changed, 61 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 54714bc7bbb8ce..a5fb612693b8f8 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -1284,7 +1284,10 @@ java_library( java_library( name = "loading-phase-threads-option", srcs = ["runtime/LoadingPhaseThreadsOption.java"], - deps = ["//src/main/java/com/google/devtools/common/options"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", + "//src/main/java/com/google/devtools/common/options", + ], ) java_library( 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 75f10006f40343..812f571e1f70a9 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 @@ -13,22 +13,29 @@ // limitations under the License. package com.google.devtools.build.lib.runtime; +import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParsingException; +import java.util.logging.Logger; /** Defines the --loading_phase_threads option which is used by multiple commands. */ public class LoadingPhaseThreadsOption extends OptionsBase { + + private static final Logger logger = Logger.getLogger(LoadingPhaseThreadsOption.class.getName()); + @Option( name = "loading_phase_threads", - defaultValue = LoadingPhaseThreadCountConverter.DEFAULT_VALUE, + defaultValue = "auto", documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION}, converter = LoadingPhaseThreadCountConverter.class, help = "Number of parallel threads to use for the loading/analysis phase." + + " \"auto\" means to use a reasonable value derived from the machine's hardware profile" + + " (e.g. the number of processors)." ) public int threads; @@ -38,33 +45,44 @@ public class LoadingPhaseThreadsOption extends OptionsBase { */ public static final class LoadingPhaseThreadCountConverter implements Converter { - private static final String DEFAULT_VALUE = "default"; - // Reduce 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. - private static final int NON_TEST_THREADS = 200; - private static final int TEST_THREADS = 20; - @Override public Integer convert(String input) throws OptionsParsingException { - if (DEFAULT_VALUE.equals(input)) { - return System.getenv("TEST_TMPDIR") == null ? NON_TEST_THREADS : TEST_THREADS; + int threads; + if (input.equals("auto")) { + threads = (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getCpuUsage()); + logger.info("Flag \"loading_phase_threads\" was set to \"auto\"; using " + threads + + " threads"); + } else { + try { + threads = Integer.decode(input); + if (threads < 1) { + throw new OptionsParsingException("'" + input + "' must be at least 1"); + } + } catch (NumberFormatException e) { + throw new OptionsParsingException("'" + input + "' is not an int"); + } } - try { - int result = Integer.decode(input); - if (result < 1) { - throw new OptionsParsingException("'" + input + "' must be at least 1"); - } - return result; - } catch (NumberFormatException e) { - throw new OptionsParsingException("'" + input + "' is not an int"); + if (System.getenv("TEST_TMPDIR") != null) { + // 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. + // + // TODO(jmmv): If tests care about this, it's them who should be setting a cap. + threads = Math.min(20, threads); + logger.info("Running under a test; loading_phase_threads capped at " + threads); } + + // 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.) + return threads; } @Override public String getTypeDescription() { - return "an integer"; + return "\"auto\" or an integer"; } } } diff --git a/src/test/shell/integration/execution_phase_tests.sh b/src/test/shell/integration/execution_phase_tests.sh index 71951a21f55f21..fe5e3bf6087920 100755 --- a/src/test/shell/integration/execution_phase_tests.sh +++ b/src/test/shell/integration/execution_phase_tests.sh @@ -249,24 +249,39 @@ function test_cache_computed_file_digests_ui() { "Digests cache not reenabled" } -function test_jobs_default_auto() { +function do_threading_default_auto_test() { + local context="${1}"; shift + local flag_name="${1}"; shift + local -r pkg="${FUNCNAME}" mkdir -p "$pkg" || fail "could not create \"$pkg\"" - # The default flag value is only read if --jobs is not set explicitly. - # Do not use a bazelrc here, this would break the test. mkdir -p $pkg/package || fail "mkdir failed" echo "cc_library(name = 'foo', srcs = ['foo.cc'])" >$pkg/package/BUILD echo "int foo(void) { return 0; }" >$pkg/package/foo.cc + # The default value of the flag under test is only read if it is not set + # explicitly. Do not use a bazelrc here as it would break the test. local output_base="$(bazel --nomaster_bazelrc --bazelrc=/dev/null info \ output_base 2>/dev/null)" || fail "bazel info should work" local java_log="${output_base}/java.log" bazel --nomaster_bazelrc --bazelrc=/dev/null build $pkg/package:foo \ >>"${TEST_log}" 2>&1 || fail "Should build" - assert_last_log "BuildRequest" 'Flag "jobs" was set to "auto"' "${java_log}" \ - "--jobs was not set to auto by default" + assert_last_log \ + "${context}" \ + "Flag \"${flag_name}\" was set to \"auto\"" \ + "${java_log}" \ + "--${flag_name} was not set to auto by default" +} + +function test_jobs_default_auto() { + do_threading_default_auto_test "BuildRequest" "jobs" +} + +function test_loading_phase_threads_default_auto() { + do_threading_default_auto_test "LoadingPhaseThreadsOption" \ + "loading_phase_threads" } function test_analysis_warning_cached() {