From 0e2a240ae178a043ddd6222057bb6ca26de842de Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 00:17:06 -0700 Subject: [PATCH 01/14] Fix run environment for executable java_binary targets Native rule classes get the run environment [for free](https://github.com/bazelbuild/bazel/blob/9709b4d1bff4e065a9fba87e26a21b51140b2920/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java#L498), Starlark rule classes must add it themselves. Fixes https://github.com/bazelbuild/bazel/issues/19115 PiperOrigin-RevId: 552391528 Change-Id: Id1b8fd6402b1d2e95ae8e4ee058d102a2b233675 --- .../starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl | 2 +- src/main/starlark/builtins_bzl/common/java/java_helper.bzl | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl index 1a5ef91e1605c3..3f1a3b85cc2c67 100644 --- a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl +++ b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl @@ -21,7 +21,7 @@ load(":common/paths.bzl", "paths") load(":common/java/java_info.bzl", "JavaInfo") def _bazel_java_binary_impl(ctx): - return _bazel_base_binary_impl(ctx, is_test_rule_class = False) + return _bazel_base_binary_impl(ctx, is_test_rule_class = False) + helper.executable_providers(ctx) def _bazel_java_test_impl(ctx): return _bazel_base_binary_impl(ctx, is_test_rule_class = True) + helper.test_providers(ctx) diff --git a/src/main/starlark/builtins_bzl/common/java/java_helper.bzl b/src/main/starlark/builtins_bzl/common/java/java_helper.bzl index 7c751f4e8ff3fd..2895829093ca75 100644 --- a/src/main/starlark/builtins_bzl/common/java/java_helper.bzl +++ b/src/main/starlark/builtins_bzl/common/java/java_helper.bzl @@ -288,6 +288,11 @@ def _test_providers(ctx): return test_providers +def _executable_providers(ctx): + if ctx.attr.create_executable: + return [_builtins.toplevel.RunEnvironmentInfo(cc_helper.get_expanded_env(ctx, {}))] + return [] + def _resource_mapper(file): return "%s:%s" % ( file.path, @@ -366,6 +371,7 @@ helper = struct( runfiles_enabled = _runfiles_enabled, get_test_support = _get_test_support, test_providers = _test_providers, + executable_providers = _executable_providers, create_single_jar = _create_single_jar, shell_quote = _shell_quote, ) From 0c917255ca95b5c25ae6cf61ebe7a63c1061f73f Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 02:07:12 -0700 Subject: [PATCH 02/14] Automated rollback of commit c061e57a7004a88eeb2f84d094d9a88b56c146b6. *** Reason for rollback *** Breaking too many downstream projects: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/3202#_ *** Original change description *** Remove the deprecated `exec_tools` attribute from `genrule`. RELNOTES: The `genrule` attribute `exec_tools` is removed. It has been identical to the `tools` attribute since Bazel 6.0 and uses should be updated. PiperOrigin-RevId: 552414536 Change-Id: Ie3ff99e1b8f290ed56157c935fcf089f1a0a5165 --- .../build/lib/rules/genrule/GenRuleBase.java | 1 + .../build/lib/rules/genrule/GenRuleBaseRule.java | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java index ecbcfbfdf9de3e..2e176731dde46f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java @@ -289,6 +289,7 @@ public String toString() { protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) { return CommandHelper.builder(ruleContext) .addToolDependencies("tools") + .addToolDependencies("exec_tools") .addToolDependencies("toolchains"); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java index bfecb964386e6b..57bb1991a8fc08 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java @@ -85,6 +85,21 @@ public RuleClass build( .cfg(ExecutionTransitionFactory.createFactory()) .allowedFileTypes(FileTypeSet.ANY_FILE)) + /* + Deprecated. Use tools instead. + +

+ There was a period of time when exec_tools and tools behaved + differently, but they are now equivalent and the Blaze team will be migrating all uses of + exec_tools to tools. +

+ */ + .add( + attr("exec_tools", LABEL_LIST) + .cfg(ExecutionTransitionFactory.createFactory()) + .allowedFileTypes(FileTypeSet.ANY_FILE) + .dontCheckConstraints()) + /* A list of files generated by this rule.

From 0763dd07df1f525b81f30fc8217c300763dd8c02 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 06:03:30 -0700 Subject: [PATCH 03/14] Materialize the runfiles output manifest as a symlink instead of a copy when building with --nobuild_runfile_links. Compare with the equivalent code path for --build_runfile_links in SymlinkTreeStrategy#createOutput. PiperOrigin-RevId: 552457444 Change-Id: I8ae22a15fcdc60395dfe2a112b6dc59e0ed10236 --- .../google/devtools/build/lib/exec/SymlinkTreeHelper.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java index 379f41f28170c1..c95e51bf596ea8 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java @@ -138,12 +138,13 @@ public void createSymlinks( /** Copies the input manifest to the output manifest. */ public void copyManifest() throws ExecException { - // Pretend we created the runfiles tree by copying the manifest + // Pretend we created the runfiles tree by symlinking the output manifest to the input manifest. + Path outputManifest = getOutputManifest(); try { symlinkTreeRoot.createDirectoryAndParents(); - FileSystemUtils.copyFile(inputManifest, getOutputManifest()); + outputManifest.createSymbolicLink(inputManifest); } catch (IOException e) { - throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_MANIFEST_COPY_IO_EXCEPTION); + throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION); } } From be63eeefa9736d66f4b773bf6043958575eb02d7 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 06:15:40 -0700 Subject: [PATCH 04/14] Add explicit `execution_phase_time_in_ms` to `TimingMetrics`. PiperOrigin-RevId: 552459757 Change-Id: Iefd62dce7af2bf3c0a0802f90c3347bb2047989f --- .../buildeventstream/proto/build_event_stream.proto | 7 +++++++ .../build/lib/metrics/MetricsCollector.java | 13 ++++++++++++- .../build/lib/metrics/MetricsCollectorTest.java | 7 +++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto index e9b88ab1a72c8c..16f616dc3c0254 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto @@ -965,6 +965,9 @@ message BuildMetrics { PackageMetrics package_metrics = 4; message TimingMetrics { + // For Skymeld, + // analysis_phase_time_in_ms + execution_phase_time_in_ms >= wall_time_in_ms + // // The CPU time in milliseconds consumed during this build. int64 cpu_time_in_ms = 1; // The elapsed wall time in milliseconds during this build. @@ -973,6 +976,10 @@ message BuildMetrics { // When analysis and execution phases are interleaved, this measures the // elapsed time from the first analysis work to the last. int64 analysis_phase_time_in_ms = 3; + // The elapsed wall time in milliseconds during the execution phase. + // When analysis and execution phases are interleaved, this measures the + // elapsed time from the first action execution to the last. + int64 execution_phase_time_in_ms = 4; } TimingMetrics timing_metrics = 5; diff --git a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java index 450f1eb4ef8335..f2b4471b05ae6c 100644 --- a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java +++ b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java @@ -99,6 +99,7 @@ class MetricsCollector { // TopLevelTargetExecutionStartedEvent. This AtomicBoolean is so that we only account for the // build once. private final AtomicBoolean buildAccountedFor; + private long executionStartMillis; @CanIgnoreReturnValue private MetricsCollector( @@ -157,6 +158,10 @@ public void onAnalysisPhaseComplete(AnalysisPhaseCompleteEvent event) { } } + private void markExecutionPhaseStarted() { + executionStartMillis = BlazeClock.instance().currentTimeMillis(); + } + @SuppressWarnings("unused") @Subscribe public synchronized void logAnalysisGraphStats(AnalysisGraphStatsEvent event) { @@ -175,13 +180,17 @@ public synchronized void logAnalysisGraphStats(AnalysisGraphStatsEvent event) { @SuppressWarnings("unused") @Subscribe public synchronized void logExecutionStartingEvent(ExecutionStartingEvent event) { + markExecutionPhaseStarted(); numBuilds.getAndIncrement(); } + // Skymeld-specific: we don't have an ExecutionStartingEvent for skymeld, so we have to use + // TopLevelTargetExecutionStartedEvent @Subscribe - public synchronized void accountForBuild( + public synchronized void handleExecutionPhaseStart( @SuppressWarnings("unused") TopLevelTargetPendingExecutionEvent event) { if (buildAccountedFor.compareAndSet(/*expectedValue=*/ false, /*newValue=*/ true)) { + markExecutionPhaseStarted(); numBuilds.getAndIncrement(); } } @@ -255,6 +264,8 @@ public void actionResultReceived(ActionResultReceivedEvent event) { @SuppressWarnings("unused") @Subscribe public void onExecutionComplete(ExecutionFinishedEvent event) { + timingMetrics.setExecutionPhaseTimeInMs( + BlazeClock.instance().currentTimeMillis() - executionStartMillis); artifactMetrics .setSourceArtifactsRead(event.sourceArtifactsRead()) .setOutputArtifactsSeen(event.outputArtifactsSeen()) diff --git a/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java b/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java index feeb64338dbb0b..14ca0f2cb013d7 100644 --- a/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java +++ b/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java @@ -519,6 +519,13 @@ public void testAnalysisTimeInMs() throws Exception { assertThat(buildMetrics.getTimingMetrics().getAnalysisPhaseTimeInMs()).isGreaterThan(0); } + @Test + public void testExecutionTimeInMs() throws Exception { + buildTarget("//foo:foo"); + BuildMetrics buildMetrics = buildMetricsEventListener.event.getBuildMetrics(); + assertThat(buildMetrics.getTimingMetrics().getExecutionPhaseTimeInMs()).isGreaterThan(0); + } + @Test public void testUsedHeapSizePostBuild() throws Exception { // TODO(bazel-team): Fix recording used heap size on Windows. From cbbe54bb52353d5ec683126907cd8e5c9ec835ca Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 06:59:07 -0700 Subject: [PATCH 05/14] Rename PROCESS_TIME to REMOTE_PROCESS_TIME to reflect the fact that it's used to mark remote execution wall time. Currently, PROCESS_TIME is used in LocalSpawnRunner to record the local execution time, but displayed as remote execution time in the Profiler which is confusing. This change reduces the chance PROCESS_TIME is misused in the future and a following change will fix LocalSpawnRunner. PiperOrigin-RevId: 552467817 Change-Id: I4383be640571fd816fb2c71fec29f60e693d4347 --- .../devtools/build/lib/exec/local/LocalSpawnRunner.java | 3 ++- .../com/google/devtools/build/lib/profiler/ProfilerTask.java | 2 +- .../google/devtools/build/lib/remote/RemoteSpawnRunner.java | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java index 3d9ce428385932..cbcc6e7d997ba5 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java @@ -425,7 +425,8 @@ private SpawnResult start() TerminationStatus terminationStatus; try (SilentCloseable c = Profiler.instance() - .profile(ProfilerTask.PROCESS_TIME, spawn.getResourceOwner().getMnemonic())) { + .profile( + ProfilerTask.REMOTE_PROCESS_TIME, spawn.getResourceOwner().getMnemonic())) { needCleanup = true; Subprocess subprocess = subprocessBuilder.start(); try { diff --git a/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java b/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java index f317a3ffe8a52d..8733b86cbe2477 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java +++ b/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java @@ -38,7 +38,7 @@ public enum ProfilerTask { Threshold.FIFTY_MILLIS, /* collectsSlowestInstances= */ true), UPLOAD_TIME("Remote execution upload time", Threshold.FIFTY_MILLIS), - PROCESS_TIME("Remote execution process wall time", Threshold.FIFTY_MILLIS), + REMOTE_PROCESS_TIME("Remote execution process wall time", Threshold.FIFTY_MILLIS), REMOTE_QUEUE("Remote execution queuing time", Threshold.FIFTY_MILLIS), REMOTE_SETUP("Remote execution setup", Threshold.FIFTY_MILLIS), FETCH("Remote execution file fetching", Threshold.FIFTY_MILLIS), diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 4d82a88c60191c..fbc44c59550bff 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -15,9 +15,9 @@ package com.google.devtools.build.lib.remote; import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.devtools.build.lib.profiler.ProfilerTask.PROCESS_TIME; import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_DOWNLOAD; import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_EXECUTION; +import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_PROCESS_TIME; import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_QUEUE; import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_SETUP; import static com.google.devtools.build.lib.profiler.ProfilerTask.UPLOAD_TIME; @@ -342,7 +342,7 @@ private static void profileAccounting(ExecutedActionMetadata executedActionMetad converter, executedActionMetadata.getExecutionStartTimestamp(), executedActionMetadata.getExecutionCompletedTimestamp(), - PROCESS_TIME, + REMOTE_PROCESS_TIME, "execute"); logProfileTask( converter, From 425e2d7457ac749aaf3873d64edf106450838759 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 07:08:35 -0700 Subject: [PATCH 06/14] Make `genrule.exec_tools` only exist in Bazel. This is a step towards removing it entirely, see #19132. Next step is to add an actual incompatible flag if possible. RELNOTES: The `genrule` attribute `exec_tools` will be removed in a future Bazel release. Please follow directions at #19132 to migrate away from it. PiperOrigin-RevId: 552469827 Change-Id: Ica480e1113a1b8aeae31ad0713c41660416248b6 --- .../build/lib/bazel/rules/genrule/BUILD | 1 + .../lib/bazel/rules/genrule/BazelGenRule.java | 12 ++++++++++++ .../bazel/rules/genrule/BazelGenRuleRule.java | 19 +++++++++++++++++++ .../build/lib/rules/genrule/GenRuleBase.java | 1 - .../lib/rules/genrule/GenRuleBaseRule.java | 15 --------------- 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD index a36517048f7c6c..2b791a509d5363 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD @@ -20,5 +20,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/rules/genrule", + "//src/main/java/com/google/devtools/build/lib/util:filetype", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java index 967e8fd7801101..ab47564cfb9d49 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.bazel.rules.genrule; +import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.rules.genrule.GenRuleBase; @@ -30,4 +31,15 @@ protected boolean isStampingEnabled(RuleContext ruleContext) { } return ruleContext.attributes().get("stamp", Type.BOOLEAN); } + + // TODO(https://github.com/bazelbuild/bazel/issues/19132): Remove this override once downstream + // projects are migrated. + @Override + protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) { + return CommandHelper.builder(ruleContext) + .addToolDependencies("tools") + // TODO(https://github.com/bazelbuild/bazel/issues/19132): Add an actual incompatible flag. + .addToolDependencies("exec_tools") + .addToolDependencies("toolchains"); + } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java index 685925c287ff7c..5055b89fa2f0a9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java @@ -15,6 +15,7 @@ import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.packages.BuildType.LABEL; +import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; import static com.google.devtools.build.lib.packages.Type.BOOLEAN; import com.google.devtools.build.lib.analysis.RuleDefinition; @@ -22,6 +23,7 @@ import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.rules.genrule.GenRuleBaseRule; +import com.google.devtools.build.lib.util.FileTypeSet; /** * Rule definition for genrule for Bazel. @@ -44,6 +46,23 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .cfg(ExecutionTransitionFactory.createFactory()) .value(env.getToolsLabel(GENRULE_SETUP_LABEL))) + // TODO(https://github.com/bazelbuild/bazel/issues/19132): Remove this once downstream + // projects are migrated. + /* + Deprecated. Use tools instead. + +

+ There was a period of time when exec_tools and tools behaved + differently, but they are now equivalent and the Blaze team will be migrating all uses of + exec_tools to tools. +

+ */ + .add( + attr("exec_tools", LABEL_LIST) + .cfg(ExecutionTransitionFactory.createFactory()) + .allowedFileTypes(FileTypeSet.ANY_FILE) + .dontCheckConstraints()) + // TODO(bazel-team): stamping doesn't seem to work. Fix it or remove attribute. .add(attr("stamp", BOOLEAN).value(false)) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java index 2e176731dde46f..ecbcfbfdf9de3e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java @@ -289,7 +289,6 @@ public String toString() { protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) { return CommandHelper.builder(ruleContext) .addToolDependencies("tools") - .addToolDependencies("exec_tools") .addToolDependencies("toolchains"); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java index 57bb1991a8fc08..bfecb964386e6b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java @@ -85,21 +85,6 @@ public RuleClass build( .cfg(ExecutionTransitionFactory.createFactory()) .allowedFileTypes(FileTypeSet.ANY_FILE)) - /* - Deprecated. Use tools instead. - -

- There was a period of time when exec_tools and tools behaved - differently, but they are now equivalent and the Blaze team will be migrating all uses of - exec_tools to tools. -

- */ - .add( - attr("exec_tools", LABEL_LIST) - .cfg(ExecutionTransitionFactory.createFactory()) - .allowedFileTypes(FileTypeSet.ANY_FILE) - .dontCheckConstraints()) - /* A list of files generated by this rule.

From 4441cef42eb332dd20c891ee856b7c807d3864be Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 07:35:30 -0700 Subject: [PATCH 07/14] Remove nodes from the in-memory graph while cleaning up interning pools. This reduces temporary memory use from objects being stored in both the weak interner and the in-memory graph. On an example large build, I observed that it did not have an impact on the time taken to perform the cleanup. Also update method names and javadoc for consistency. PiperOrigin-RevId: 552476051 Change-Id: Ide5d7f631d7139e3d169110ece75eb3c066f2702 --- .../AbstractInMemoryMemoizingEvaluator.java | 2 +- .../devtools/build/skyframe/InMemoryGraph.java | 8 +++++--- .../build/skyframe/InMemoryGraphImpl.java | 18 ++++++------------ .../build/skyframe/MemoizingEvaluator.java | 7 +++++-- .../cmdline/LabelInternerIntegrationTest.java | 2 +- .../skyframe/DeterministicInMemoryGraph.java | 4 ++-- .../build/skyframe/InMemoryGraphTest.java | 2 +- .../build/skyframe/NotifyingInMemoryGraph.java | 4 ++-- 8 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java index 0a9215a1b5e440..aab1ab2eddad06 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java @@ -161,6 +161,6 @@ private static String canonicalizeKey(SkyKey key) { @Override public void cleanupInterningPools() { - getInMemoryGraph().cleanupInterningPool(); + getInMemoryGraph().cleanupInterningPools(); } } diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java index e2849dbd991570..83d28c414acbaa 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java +++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java @@ -92,10 +92,12 @@ default int valuesSize() { void removeIfDone(SkyKey key); /** - * Cleans up the {@link com.google.devtools.build.lib.concurrent.PooledInterner.Pool} by moving - * instances back to weak interner and uninstall current pool. + * Cleans up {@linkplain com.google.devtools.build.lib.concurrent.PooledInterner.Pool interning + * pools} by moving objects to weak interners and uninstalling the current pools. + * + *

May destroy this graph. Only call when the graph is about to be thrown away. */ - void cleanupInterningPool(); + void cleanupInterningPools(); /** * Returns the {@link InMemoryNodeEntry} for a given {@link SkyKey} if present in the graph. diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java index e1ddd3bacd922f..ac1baf5397afdb 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java +++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java @@ -233,17 +233,9 @@ public void parallelForEach(Consumer consumer) { nodeMap.forEachValue(PARALLELISM_THRESHOLD, consumer); } - /** - * Re-interns {@link SkyKey} instances that use {@link SkyKeyInterner} in node map back to the - * {@link SkyKeyInterner}'s weak interner. - * - *

Also uninstalls current {@link SkyKeyPool} instance from being {@link SkyKeyInterner}'s - * static global pool. - */ @Override - public void cleanupInterningPool() { + public void cleanupInterningPools() { if (!usePooledInterning) { - // No clean up is needed when `usePooledInterning` is false for shell integration tests. return; } try (AutoProfiler ignored = @@ -252,11 +244,13 @@ public void cleanupInterningPool() { e -> { weakInternSkyKey(e.getKey()); - if (!e.isDone() || !e.getKey().functionName().equals(SkyFunctions.PACKAGE)) { - return; + if (e.isDone() && e.getKey().functionName().equals(SkyFunctions.PACKAGE)) { + weakInternPackageTargetsLabels((PackageValue) e.toValue()); } - weakInternPackageTargetsLabels((PackageValue) e.toValue()); + // The graph is about to be thrown away. Remove as we go to avoid temporarily storing + // everything in both the weak interner and the graph. + nodeMap.remove(e.getKey()); }); } diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java index ccf1a15117686f..603a9ed7d62b52 100644 --- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java @@ -203,8 +203,11 @@ public ProcessableGraph transform(ProcessableGraph graph) { void dumpRdeps(PrintStream out, Predicate filter) throws InterruptedException; /** - * Cleans up the pool when {@link InMemoryGraph} serves as an alternative global pool to weak - * interner for interning {@link SkyKey} and {@link com.google.devtools.build.lib.cmdline.Label}. + * Cleans up {@linkplain com.google.devtools.build.lib.concurrent.PooledInterner.Pool interning + * pools} by moving objects to weak interners and uninstalling the current pools. + * + *

May destroy this evaluator's {@linkplain #getInMemoryGraph graph}. Only call when the graph + * is about to be thrown away. */ void cleanupInterningPools(); } diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java index 41f0ef54aeaa29..eaee62da410146 100644 --- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java @@ -219,6 +219,6 @@ public void labelInterner_alwaysRespectWeakInternerWhenLabelInternerDisabled() t @After public void cleanup() { - skyframeExecutor().getEvaluator().getInMemoryGraph().cleanupInterningPool(); + skyframeExecutor().getEvaluator().getInMemoryGraph().cleanupInterningPools(); } } diff --git a/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java index 31b534c67a75d9..1b5bade699565f 100644 --- a/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java +++ b/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java @@ -80,8 +80,8 @@ public void parallelForEach(Consumer consumer) { } @Override - public void cleanupInterningPool() { - ((InMemoryGraph) delegate).cleanupInterningPool(); + public void cleanupInterningPools() { + ((InMemoryGraph) delegate).cleanupInterningPools(); } @Override diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java index 8ec297fbd0af27..339b4d28bc6163 100644 --- a/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java @@ -121,7 +121,7 @@ public void cleanupPool_weakInternerReintern() throws InterruptedException { assertThat(graph.get(null, Reason.OTHER, cat)).isNotNull(); assertThat(graph).isInstanceOf(InMemoryGraphImpl.class); - ((InMemoryGraphImpl) graph).cleanupInterningPool(); + ((InMemoryGraphImpl) graph).cleanupInterningPools(); // When re-creating a cat SkyKeyWithSkyKeyInterner, we expect to get the original instance. Pool // cleaning up re-interns the cat instance back to the weak interner, and thus, no new instance diff --git a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java index e9ba9568b711fe..ad35a7f513d9b8 100644 --- a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java +++ b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java @@ -87,8 +87,8 @@ public void parallelForEach(Consumer consumer) { } @Override - public void cleanupInterningPool() { - ((InMemoryGraph) delegate).cleanupInterningPool(); + public void cleanupInterningPools() { + ((InMemoryGraph) delegate).cleanupInterningPools(); } @Override From ff83807a8e792c5d8608464d6a17127562165680 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 07:38:58 -0700 Subject: [PATCH 08/14] [Skymeld] Fix the flaky `testTwoConflictingTargets_keepGoing_behaviorDifferences` Consider 2 targets `//foo` and `//foo/bar` which generates 2 artifacts `foo` and `foo/bar`. In skymeld and `-k` mode, it's possible that either 1 of the 2 artifacts are built or none at all. The last case can happen when the `ArtifactPrefixConflictException` are thrown for both the evaluation of `BuildDriverKey(//foo)` and `BuildDriverKey(//foo/bar)`, which is possible given that the `IncrementalArtfiactConflictFinder` can be accessed from multiple threads. This CL updates the test to reflect that. PiperOrigin-RevId: 552476900 Change-Id: Ifc67a9f1c09ab1887656ea491a6f2247484163e2 --- .../lib/buildtool/OutputArtifactConflictTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java index 18f06729ec0901..00abaf8a379d96 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java @@ -661,10 +661,11 @@ public void testConflictAfterNullBuild( assertThat(eventListener.eventIds.get(0).getAspect()).isEqualTo("//x:aspect.bzl%my_aspect"); } - // There exists a discrepancy between --experimental_merged_skyframe_analysis_execution and - // otherwise in case of --keep_going. The version with merged phases would still build one of the - // 2 conflicting targets, while the one without would stop at the end of the analysis phase and - // build nothing. The overall build would still fail. + // There exists a discrepancy between skymeld and noskymeld modes in case of --keep_going. + // noskymeld: bazel would stop at the end of the analysis phase and build nothing. + // skymeld: we either finish building one of the 2 conflicting artifacts, or none at all. + // + // The overall build would still fail in both cases. @Test public void testTwoConflictingTargets_keepGoing_behaviorDifferences( @TestParameter boolean mergedAnalysisExecution) throws Exception { @@ -679,8 +680,8 @@ public void testTwoConflictingTargets_keepGoing_behaviorDifferences( Path outputXYY = Iterables.getOnlyElement(getArtifacts("//x/y:y")).getPath(); if (mergedAnalysisExecution) { - // Verify that one and only one of the output artifacts from these 2 targets were built. - assertThat((outputXY.isDirectory() && outputXYY.isFile()) ^ outputXY.isFile()).isTrue(); + // Verify that these 2 conflicting artifacts can't both exist. + assertThat(outputXYY.isFile() && outputXY.isFile()).isFalse(); } else { // Verify that none of the output artifacts were built. assertThat(outputXY.exists()).isFalse(); From dd6393a7e9e8a70cc1a2554c34aec056d155fe5b Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 09:01:15 -0700 Subject: [PATCH 09/14] Add `--incompatible_remove_exec_tools` to control whether genrule has an `exec_tools` attribute. Part of #19132. PiperOrigin-RevId: 552496824 Change-Id: I2a09c390da89a44506afae55319aa393ed292c62 --- .../build/lib/bazel/BazelConfiguration.java | 10 ++++++++++ .../devtools/build/lib/bazel/rules/genrule/BUILD | 1 + .../build/lib/bazel/rules/genrule/BazelGenRule.java | 13 ++++++++++++- 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java index bb04d748267163..8f0a8ddac750fa 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java @@ -40,10 +40,20 @@ public static class Options extends FragmentOptions { help = "If enabled, visibility checking also applies to toolchain implementations.") public boolean checkVisibilityForToolchains; + @Option( + name = "incompatible_remove_exec_tools", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = "If enabled, use of genrule's exec_tools attribute will cause an error..") + public boolean removeExecTools; + @Override public FragmentOptions getExec() { Options exec = (Options) getDefault(); exec.checkVisibilityForToolchains = checkVisibilityForToolchains; + exec.removeExecTools = removeExecTools; return exec; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD index 2b791a509d5363..c516cdc1984b57 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD @@ -18,6 +18,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", + "//src/main/java/com/google/devtools/build/lib/bazel:bazel_configuration", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/rules/genrule", "//src/main/java/com/google/devtools/build/lib/util:filetype", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java index ab47564cfb9d49..5deff13fd3edc3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java @@ -16,6 +16,8 @@ import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.bazel.BazelConfiguration; +import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.rules.genrule.GenRuleBase; @@ -36,9 +38,18 @@ protected boolean isStampingEnabled(RuleContext ruleContext) { // projects are migrated. @Override protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) { + BazelConfiguration.Options bazelOptions = + ruleContext.getConfiguration().getOptions().get(BazelConfiguration.Options.class); + + if (bazelOptions.removeExecTools + && ruleContext.attributes().has("exec_tools", BuildType.LABEL_LIST) + && !ruleContext.attributes().get("exec_tools", BuildType.LABEL_LIST).isEmpty()) { + ruleContext.attributeError( + "exec_tools", "genrule.exec_tools has been removed, use tools instead"); + } + return CommandHelper.builder(ruleContext) .addToolDependencies("tools") - // TODO(https://github.com/bazelbuild/bazel/issues/19132): Add an actual incompatible flag. .addToolDependencies("exec_tools") .addToolDependencies("toolchains"); } From aef8d9d22d3bccfd86abcc9e09ad4b8349f37c70 Mon Sep 17 00:00:00 2001 From: Bazel Release System Date: Mon, 31 Jul 2023 16:09:26 +0000 Subject: [PATCH 10/14] Release 6.3.1 (2023-07-31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Baseline: 0f231ac8acabcd8aa309da041c98ab90a1552418 Release Notes: + Mark isolated extension usages as experimental (#19050) + Fix a bug where frozen targets list was mutated while expanding env attribute (#19052) + Add documentation for --experimental_isolated_extension_usage (#19071) + Advertise CcInfo from cc_import (#19086) + Create .bazelversion to address postsubmit issues (#19089) + Update java_tools version to 12.6 (#19091) + Disable lockfiles by default (#19106) Acknowledgements: This release contains contributions from many people at Google, as well as Brentley Jones, Fabian Meumertzheim, oquenchil, Xùdōng Yáng. --- CHANGELOG.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5edd7120030ee..4c33131a7c7d9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,23 @@ +## Release 6.3.1 (2023-07-31) + +``` +Baseline: 0f231ac8acabcd8aa309da041c98ab90a1552418 + +Release Notes: + ++ Mark isolated extension usages as experimental (#19050) ++ Fix a bug where frozen targets list was mutated while expanding env attribute (#19052) ++ Add documentation for --experimental_isolated_extension_usage (#19071) ++ Advertise CcInfo from cc_import (#19086) ++ Create .bazelversion to address postsubmit issues (#19089) ++ Update java_tools version to 12.6 (#19091) ++ Disable lockfiles by default (#19106) + +Acknowledgements: + +This release contains contributions from many people at Google, as well as Brentley Jones, Fabian Meumertzheim, oquenchil, Xùdōng Yáng. +``` + ## Release 7.0.0-pre.20230710.5 (2023-07-28) ``` From 87bd420a2a0ee863c073f903d2f7b2eccb5940d6 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 12:10:10 -0700 Subject: [PATCH 11/14] Allow variables_extension to be "None" This is a nicer interface than emitting an error on "None" input. PiperOrigin-RevId: 552554850 Change-Id: I799f4d0baeb8ce0f1b2a320ff2261559f5e4572e --- .../devtools/build/lib/rules/objc/AppleStarlarkCommon.java | 2 +- .../build/lib/starlarkbuildapi/objc/AppleCommonApi.java | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java index 4db5b02d96ebb7..a149f17500ca80 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java @@ -252,7 +252,7 @@ public AppleExecutableBinaryInfo newExecutableBinaryProvider( } private Dict asDict(Object o) { - return o == Starlark.UNBOUND ? Dict.empty() : (Dict) o; + return o == Starlark.NONE ? Dict.empty() : (Dict) o; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java index b09857d5695e58..8312046092b981 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java @@ -420,8 +420,11 @@ AppleExecutableBinaryApi newExecutableBinaryProvider( positional = false, named = true, documented = false, - allowedTypes = {@ParamType(type = Dict.class)}, - defaultValue = "unbound"), + allowedTypes = { + @ParamType(type = Dict.class), + @ParamType(type = NoneType.class), + }, + defaultValue = "None"), }, useStarlarkThread = true) // TODO(b/70937317): Iterate on, improve, and solidify this API. From db579e442db43ca28d892f934abe185c534bd689 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 31 Jul 2023 12:22:26 -0700 Subject: [PATCH 12/14] Update the .bazelversion and support matrix for 6.3.1 PiperOrigin-RevId: 552558168 Change-Id: I2c10ddbd8c7e1adb5a9889ac30a713fa58933999 --- .bazelversion | 2 +- site/en/release/index.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.bazelversion b/.bazelversion index 798e38995c4d67..dc0208aba8e455 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -6.3.0 +6.3.1 diff --git a/site/en/release/index.md b/site/en/release/index.md index d9375860d8280a..e2773cd9d2430a 100644 --- a/site/en/release/index.md +++ b/site/en/release/index.md @@ -84,7 +84,7 @@ on Github. | LTS release | Support stage | Latest version | End of support | | ----------- | ------------- | -------------- | -------------- | | Bazel 7 | Rolling| [Check GitHub release page](https://github.com/bazelbuild/bazel/releases){: .external} | N/A | -| Bazel 6 | Active | [6.3.0](https://github.com/bazelbuild/bazel/releases/tag/6.3.0){: .external} | Dec 2025 | +| Bazel 6 | Active | [6.3.1](https://github.com/bazelbuild/bazel/releases/tag/6.3.1){: .external} | Dec 2025 | | Bazel 5 | Maintenance | [5.4.1](https://github.com/bazelbuild/bazel/releases/tag/5.4.1){: .external} | Jan 2025 | | Bazel 4 | Maintenance | [4.2.4](https://github.com/bazelbuild/bazel/releases/tag/4.2.4){: .external} | Jan 2024 | From 1997f09a83d8bb91247617afeb199381b5c03cc4 Mon Sep 17 00:00:00 2001 From: Matt Mackay Date: Mon, 31 Jul 2023 13:26:50 -0700 Subject: [PATCH 13/14] feat: add option to exit early if analysis cache is discarded Adds `--allow_analysis_cache_discard` option that allows the build to exit early if the analysis cache would have been discarded. The flag name is of course open to bikeshedding etc. fixes #16804 Closes #16805. PiperOrigin-RevId: 552575951 Change-Id: Ia336eb3a5b7d7e41665fd0e0adf3edc03ed50f18 --- .../build/lib/analysis/AnalysisOptions.java | 11 +++++++++ .../build/lib/analysis/BuildView.java | 7 ++++-- .../devtools/build/lib/buildeventstream/BUILD | 1 + .../BuildCompletingEvent.java | 24 +++++++++++++++---- .../proto/build_event_stream.proto | 3 +++ .../buildevent/BuildCompleteEvent.java | 2 +- .../build/lib/skyframe/SkyframeBuildView.java | 14 ++++++++++- src/main/protobuf/failure_details.proto | 2 ++ .../lib/analysis/AnalysisCachingTest.java | 13 ++++++++++ .../google/devtools/build/lib/analysis/BUILD | 1 + .../analysis/util/BuildViewForTesting.java | 13 +++++++++- 11 files changed, 82 insertions(+), 9 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 5ccce705a0a798..dd2014011e8c26 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 @@ -39,6 +39,17 @@ public class AnalysisOptions extends OptionsBase { ) public boolean discardAnalysisCache; + @Option( + name = "allow_analysis_cache_discard", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.EAGERNESS_TO_EXIT}, + help = + "If discarding the analysis cache due to a change in the build system, setting this" + + " option to false will cause bazel to exit, rather than continuing with the build." + + " This option has no effect when 'discard_analysis_cache' is also set.") + public boolean allowAnalysisCacheDiscards; + @Option( name = "max_config_changes_to_show", defaultValue = "3", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index a0e9892b3e45a8..c3d2c9e6770b1b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -255,7 +255,10 @@ public AnalysisResult update( } skyframeBuildView.setConfiguration( - eventHandler, topLevelConfig, viewOptions.maxConfigChangesToShow); + eventHandler, + topLevelConfig, + viewOptions.maxConfigChangesToShow, + viewOptions.allowAnalysisCacheDiscards); eventBus.post(new MakeEnvironmentEvent(topLevelConfig.getMakeEnvironment())); eventBus.post(topLevelConfig.toBuildEvent()); @@ -510,7 +513,7 @@ private AnalysisResult createResult( ImmutableMap labelToTargetMap, boolean includeExecutionPhase) throws InterruptedException { - Set

After completing a task, the worker checks if there are any available tasks that it may * execute, subject to CPU permit constraints. On finding and reserving an appropriate task, the - * worker returns its next planned activity, {@link #IDLE} or {@link #QUIESCENT} if it finds - * nothing to do. + * worker returns its next planned activity, {@link #IDLE} if it finds nothing to do. */ enum NextWorkerActivity { - /** The worker will stop and is the last worker working. */ - QUIESCENT, /** The worker will stop. */ IDLE, /** The worker will perform a non-CPU heavy task. */ @@ -386,12 +389,8 @@ private WorkerThread(ForkJoinPool pool, String name) { private void runLoop(NextWorkerActivity nextActivity) { while (true) { switch (nextActivity) { - case QUIESCENT: - synchronized (quiescenceMonitor) { - quiescenceMonitor.notifyAll(); - } - return; case IDLE: + workerBecomingIdle(); return; case DO_TASK: dequeueTaskAndRun(); @@ -404,14 +403,6 @@ private void runLoop(NextWorkerActivity nextActivity) { } } } - - boolean tryDoQueuedWork() { - if (!tryAcquireTask()) { - return false; - } - dequeueTaskAndRun(); - return true; - } } private void dequeueTaskAndRun() { @@ -431,6 +422,14 @@ private void dequeueCpuHeavyTaskAndRun() { } } + private void workerBecomingIdle() { + if (UNSAFE.getAndAddInt(null, activeWorkerCountAddress, -1) == 1) { + synchronized (quiescenceMonitor) { + quiescenceMonitor.notifyAll(); + } + } + } + // The constants below apply to the 64-bit execution counters value. private static final long CANCEL_BIT = 0x8000_0000_0000_0000L; @@ -479,10 +478,10 @@ private void dequeueCpuHeavyTaskAndRun() { */ private final long countersAddress; + private final long activeWorkerCountAddress; + boolean isQuiescent() { - long snapshot = getExecutionCounters(); - int threadsSnapshot = (int) ((snapshot & THREADS_MASK) >> THREADS_BIT_OFFSET); - return threadsSnapshot == poolSize; + return UNSAFE.getInt(null, activeWorkerCountAddress) == 0; } boolean isCancelled() { @@ -505,6 +504,7 @@ private void resetExecutionCounters() { countersAddress, (((long) poolSize) << THREADS_BIT_OFFSET) | (((long) cpuPermits) << CPU_PERMITS_BIT_OFFSET)); + UNSAFE.putInt(null, activeWorkerCountAddress, 0); } private boolean acquireThreadElseReleaseTask() { @@ -532,17 +532,6 @@ private boolean acquireThreadAndCpuPermitElseReleaseCpuHeavyTask() { } while (true); } - private boolean tryAcquireTask() { - long snapshot; - do { - snapshot = getExecutionCounters(); - if ((snapshot & TASKS_MASK) == 0 || snapshot < 0) { - return false; - } - } while (!tryUpdateExecutionCounters(snapshot, snapshot - ONE_TASK)); - return true; - } - /** * Worker threads determine their next action after completing a task using this method. * @@ -564,7 +553,7 @@ private NextWorkerActivity getActivityFollowingTask() { } else { long target = snapshot + ONE_THREAD; if (UNSAFE.compareAndSwapLong(null, countersAddress, snapshot, target)) { - return quiescentOrIdle(target); + return IDLE; } } snapshot = UNSAFE.getLong(null, countersAddress); @@ -574,8 +563,8 @@ private NextWorkerActivity getActivityFollowingTask() { /** * Worker threads call this to determine their next action after completing a CPU heavy task. * - *

This releases a CPU permit when returning {@link NextWorkerActivity#QUIESCENT}, {@link - * NextWorkerActivity#IDLE} or {@link NextWorkerActivity#DO_TASK}. + *

This releases a CPU permit when returning {@link NextWorkerActivity#IDLE} or {@link + * NextWorkerActivity#DO_TASK}. */ private NextWorkerActivity getActivityFollowingCpuHeavyTask() { long snapshot = UNSAFE.getLongVolatile(null, countersAddress); @@ -593,18 +582,13 @@ private NextWorkerActivity getActivityFollowingCpuHeavyTask() { } else { long target = snapshot + CPU_HEAVY_RESOURCES; if (UNSAFE.compareAndSwapLong(null, countersAddress, snapshot, target)) { - return quiescentOrIdle(target); + return IDLE; } } snapshot = UNSAFE.getLong(null, countersAddress); } while (true); } - private NextWorkerActivity quiescentOrIdle(long snapshot) { - int snapshotThreads = (int) ((snapshot & THREADS_MASK) >> THREADS_BIT_OFFSET); - return snapshotThreads == poolSize ? QUIESCENT : IDLE; - } - // Throughout this class, the following wrappers are used where possible, but they are often not // inlined by the JVM even though they show up on profiles, so they are inlined explicitly in // numerous cases. diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java b/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java index 1d1912a5759094..3aa135b6f565cc 100644 --- a/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java @@ -16,7 +16,6 @@ import static com.google.common.base.MoreObjects.toStringHelper; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Throwables.throwIfUnchecked; -import static java.lang.Thread.currentThread; import com.google.common.annotations.VisibleForTesting; import com.google.common.util.concurrent.ListenableFuture; @@ -41,11 +40,6 @@ * *

The queue for non-CPUHeavy tasks has a fixed capacity. When full, callers of execute assist * with enqueued work. - * - *

Threads may voluntarily assist with queued work by calling {@link - * TieredPriorityExecutor#tryDoQueuedWork} when a thread is about to block. If tasks are available, - * the current thread may perform a task inside {@link TieredPriorityExecutor#tryDoQueuedWork} - * before it returns. This may add latency to the donating thread but can reduce overhead. */ public final class TieredPriorityExecutor implements QuiescingExecutor { /** A common cleaner shared by all executors. */ @@ -163,23 +157,6 @@ public CountDownLatch getInterruptionLatchForTestingOnly() { throw new UnsupportedOperationException(); } - /** - * Attempts to donate work on the current thread. - * - *

Calling this method may be useful if the current thread is about to block. Subject to - * scheduling constraints, attempts to poll work from the queue and execute it on the current - * thread. - * - * @return true if work was donated, false otherwise. - */ - public static boolean tryDoQueuedWork() { - var thread = currentThread(); - if (!(thread instanceof PriorityWorkerPool.WorkerThread)) { - return false; - } - return ((PriorityWorkerPool.WorkerThread) thread).tryDoQueuedWork(); - } - /** * The parallelism target of the underlying thread pool. * diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java b/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java index bac6320756053c..0809fe55875646 100644 --- a/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java @@ -36,7 +36,6 @@ import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -493,94 +492,6 @@ public void criticalError_disposesPool() throws InterruptedException { } while (true); } - @Test - public void workDonation_processesAllTasks() throws InterruptedException { - var holdAllThreads = new CountDownLatch(1); - for (int i = 0; i < POOL_SIZE - 1; ++i) { - executor.execute(() -> awaitUninterruptibly(holdAllThreads)); - } - - var donor = new AtomicReference(); - var donorSet = new CountDownLatch(1); - var gate = new CountDownLatch(1); - var donationDone = new CountDownLatch(1); - executor.execute( - () -> { - donor.set(Thread.currentThread()); - donorSet.countDown(); - awaitUninterruptibly(gate); - for (int i = 0; i < 100; ++i) { - assertThat(TieredPriorityExecutor.tryDoQueuedWork()).isTrue(); - } - donationDone.countDown(); - awaitUninterruptibly(holdAllThreads); - }); - - var receiver = new AtomicInteger(); - for (int i = 0; i < 100; ++i) { - executor.execute( - () -> { - // This task is running from the donor's thread. - assertThat(Thread.currentThread()).isEqualTo(donor.get()); - receiver.getAndIncrement(); - }); - } - - donorSet.await(); - gate.countDown(); - donationDone.await(); - assertThat(receiver.get()).isEqualTo(100); - holdAllThreads.countDown(); - executor.awaitQuiescence(/* interruptWorkers= */ true); - } - - @Test - public void workDonation_handlesErrorsInDonatedWork() throws InterruptedException { - var interruptedCount = new AtomicInteger(); - var holdAllThreads = new CountDownLatch(1); - for (int i = 0; i < POOL_SIZE - 1; ++i) { - executor.execute( - () -> { - try { - while (!holdAllThreads.await(INTERRUPT_POLL_MS, MILLISECONDS)) {} - } catch (InterruptedException e) { - interruptedCount.getAndIncrement(); - } - }); - } - - var donor = new AtomicReference(); - var donorSet = new CountDownLatch(1); - var gate = new CountDownLatch(1); - executor.execute( - () -> { - donor.set(Thread.currentThread()); - donorSet.countDown(); - awaitUninterruptibly(gate); - assertThat(TieredPriorityExecutor.tryDoQueuedWork()).isTrue(); - try { - while (!holdAllThreads.await(INTERRUPT_POLL_MS, MILLISECONDS)) {} - } catch (InterruptedException e) { - interruptedCount.getAndIncrement(); - } - }); - - executor.execute( - () -> { - assertThat(Thread.currentThread()).isEqualTo(donor.get()); - throw new AssertionError("critical error"); - }); - - donorSet.await(); - gate.countDown(); - - var error = - assertThrows( - AssertionError.class, () -> executor.awaitQuiescence(/* interruptWorkers= */ true)); - assertThat(error).hasMessageThat().contains("critical error"); - assertThat(interruptedCount.get()).isEqualTo(POOL_SIZE); - } - @Test public void settableFuture_respondsToInterrupt() throws InterruptedException { var interruptedCount = new AtomicInteger(); @@ -682,31 +593,22 @@ public void taskQueueOverflow_executesTasks() throws InterruptedException { // Waits for holders to start, otherwise they might race against the filling of the queue below. allHoldersStarted.await(); - // Fills up the queue. - var executed = new ArrayList(); + // Over-fills the queue. + var executed = Sets.newConcurrentHashSet(); var expected = new ArrayList(); - for (int i = 0; i < PriorityWorkerPool.TASKS_MAX_VALUE; ++i) { + for (int i = 0; i < 2 * PriorityWorkerPool.TASKS_MAX_VALUE; ++i) { expected.add(i); final int index = i; executor.execute(() -> executed.add(index)); } - // Adds tasks that would overflow the queue. Since overflows consume tasks from the queue, this - // causes all the tasks above to be executed. - var donorValues = Sets.newConcurrentHashSet(); - for (int i = 0; i < PriorityWorkerPool.TASKS_MAX_VALUE; ++i) { - final int index = i; - executor.execute(() -> donorValues.add(index)); - } - - assertThat(executed).isEqualTo(expected); - assertThat(donorValues).isEmpty(); + assertThat(executed).isEmpty(); holdAllThreads.countDown(); executor.awaitQuiescence(/* interruptWorkers= */ true); - assertThat(donorValues).containsExactlyElementsIn(expected); + assertThat(executed).containsExactlyElementsIn(expected); } @Test