From 6bbad59aea92c7622ebdde0aa3ac27ae5bee6269 Mon Sep 17 00:00:00 2001 From: janakr Date: Wed, 17 Feb 2021 09:00:38 -0800 Subject: [PATCH] Fix a failure to detect action conflicts if the action conflict came about via a changed aspect that didn't change any configured targets. Realized that that failure also indicated we weren't properly accounting for aspects in our metrics. Made our metrics count them, and added in some additional fields that allow people to track the old thing if they care, or (more likely) to ease the transition from old to new: I saw 50% increases for some metrics when aspect data was added in. Also realized that we were storing a set of configured target keys during analysis for no good reason, and deleted that to save memory. The only casualty is the targets_loaded field, which was bogus anyway. unknown commit added it without anyone seeming to realize that it was not counting "loaded" targets, but rather "analyzed labels", which is not a particularly useful metric. RELNOTES[INC]: In the build event stream, BuildMetrics.TargetMetrics.targets_loaded is no longer populated. Its value was always mostly meaningless. BuildMetrics.TargetMetrics.targets_configured and BuildMetrics.ActionSummary.actions_created now include configured aspect data. PiperOrigin-RevId: 357959578 --- .../lib/actions/AnalysisGraphStatsEvent.java | 12 +- .../google/devtools/build/lib/actions/BUILD | 15 +++ .../TotalAndConfiguredTargetOnlyMetric.java | 34 ++++++ .../analysis/AnalysisPhaseCompleteEvent.java | 34 ++---- .../google/devtools/build/lib/analysis/BUILD | 2 +- .../build/lib/analysis/BuildView.java | 26 ++--- .../proto/build_event_stream.proto | 45 ++++++-- .../lib/buildtool/AnalysisPhaseRunner.java | 8 +- .../google/devtools/build/lib/metrics/BUILD | 1 + .../build/lib/metrics/MetricsCollector.java | 20 +++- .../lib/skyframe/ArtifactConflictFinder.java | 32 ++++-- .../google/devtools/build/lib/skyframe/BUILD | 2 + .../build/lib/skyframe/SkyframeBuildView.java | 108 ++++++++++-------- .../build/lib/skyframe/SkyframeExecutor.java | 2 +- .../build/lib/analysis/AspectTest.java | 73 ++++++++++++ .../google/devtools/build/lib/analysis/BUILD | 2 +- .../test/TrimTestConfigurationTest.java | 16 ++- .../lib/analysis/util/AnalysisTestCase.java | 8 +- .../analysis/util/BuildViewForTesting.java | 35 +++++- 19 files changed, 334 insertions(+), 141 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/TotalAndConfiguredTargetOnlyMetric.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java b/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java index 5a990183ecb0b4..45a4039811a555 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java @@ -19,22 +19,24 @@ * com.google.devtools.build.lib.skyframe.SkyframeBuildView#shouldCheckForConflicts}. */ public final class AnalysisGraphStatsEvent { - private final int actionLookupValueCount; - private final int actionCount; + private final TotalAndConfiguredTargetOnlyMetric actionLookupValueCount; + private final TotalAndConfiguredTargetOnlyMetric actionCount; private final int outputArtifactCount; public AnalysisGraphStatsEvent( - int actionLookupValueCount, int actionCount, int outputArtifactCount) { + TotalAndConfiguredTargetOnlyMetric actionLookupValueCount, + TotalAndConfiguredTargetOnlyMetric actionCount, + int outputArtifactCount) { this.actionLookupValueCount = actionLookupValueCount; this.actionCount = actionCount; this.outputArtifactCount = outputArtifactCount; } - public int getActionLookupValueCount() { + public TotalAndConfiguredTargetOnlyMetric getActionLookupValueCount() { return actionLookupValueCount; } - public int getActionCount() { + public TotalAndConfiguredTargetOnlyMetric getActionCount() { return actionCount; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 51b6dbf3bff363..da7660938253ab 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -28,6 +28,7 @@ java_library( exclude = [ "ActionInput.java", "ActionLookupKey.java", + "AnalysisGraphStatsEvent.java", "Artifact.java", "ArtifactFactory.java", "ArtifactOwner.java", @@ -135,6 +136,14 @@ java_library( ], ) +java_library( + name = "total_and_configured_target_only_metric", + srcs = ["TotalAndConfiguredTargetOnlyMetric.java"], + deps = [ + "//third_party:auto_value", + ], +) + java_library( name = "action_lookup_key", srcs = ["ActionLookupKey.java"], @@ -144,6 +153,12 @@ java_library( ], ) +java_library( + name = "analysis_graph_stats_event", + srcs = ["AnalysisGraphStatsEvent.java"], + deps = [":total_and_configured_target_only_metric"], +) + java_library( name = "artifact_owner", srcs = ["ArtifactOwner.java"], diff --git a/src/main/java/com/google/devtools/build/lib/actions/TotalAndConfiguredTargetOnlyMetric.java b/src/main/java/com/google/devtools/build/lib/actions/TotalAndConfiguredTargetOnlyMetric.java new file mode 100644 index 00000000000000..2ebd41dfbf1ff3 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/TotalAndConfiguredTargetOnlyMetric.java @@ -0,0 +1,34 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.actions; + +import com.google.auto.value.AutoValue; + +/** + * Class representing a metric for which we have both an overall number (including aspects) and + * specific-to-configured-target number. Usually aspects and configured targets should be considered + * together, but some historical metrics only counted configured targets, so we provide this + * granularity for consumers who care. + */ +@AutoValue +public abstract class TotalAndConfiguredTargetOnlyMetric { + public abstract int total(); + + public abstract int configuredTargetsOnly(); + + public static TotalAndConfiguredTargetOnlyMetric create(int total, int configuredTargetsOnly) { + return new AutoValue_TotalAndConfiguredTargetOnlyMetric(total, configuredTargetsOnly); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisPhaseCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisPhaseCompleteEvent.java index 99de3014ed2d8b..b925e1d030e2ac 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisPhaseCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisPhaseCompleteEvent.java @@ -14,9 +14,11 @@ package com.google.devtools.build.lib.analysis; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.devtools.build.lib.pkgcache.PackageManager.PackageManagerStatistics; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.TotalAndConfiguredTargetOnlyMetric; import java.util.Collection; /** @@ -26,31 +28,23 @@ public class AnalysisPhaseCompleteEvent { private final Collection topLevelTargets; private final long timeInMs; - private int targetsLoaded; - private int targetsConfigured; + private final TotalAndConfiguredTargetOnlyMetric targetsConfigured; private final PackageManagerStatistics pkgManagerStats; - private final int actionsConstructed; + private final TotalAndConfiguredTargetOnlyMetric actionsConstructed; private final boolean analysisCacheDropped; - /** - * Construct the event. - * - * @param topLevelTargets The set of active topLevelTargets that remain. - */ public AnalysisPhaseCompleteEvent( Collection topLevelTargets, - int targetsLoaded, - int targetsConfigured, + TotalAndConfiguredTargetOnlyMetric targetsConfigured, + TotalAndConfiguredTargetOnlyMetric actionsConstructed, long timeInMs, PackageManagerStatistics pkgManagerStats, - int actionsConstructed, boolean analysisCacheDropped) { this.timeInMs = timeInMs; this.topLevelTargets = ImmutableList.copyOf(topLevelTargets); - this.targetsLoaded = targetsLoaded; - this.targetsConfigured = targetsConfigured; + this.targetsConfigured = checkNotNull(targetsConfigured); this.pkgManagerStats = pkgManagerStats; - this.actionsConstructed = actionsConstructed; + this.actionsConstructed = checkNotNull(actionsConstructed); this.analysisCacheDropped = analysisCacheDropped; } @@ -62,13 +56,8 @@ public Collection getTopLevelTargets() { return topLevelTargets; } - /** Returns the number of targets loaded during analysis */ - public int getTargetsLoaded() { - return targetsLoaded; - } - - /** Returns the number of targets configured during analysis */ - public int getTargetsConfigured() { + /** Returns the number of targets/aspects configured during analysis. */ + public TotalAndConfiguredTargetOnlyMetric getTargetsConfigured() { return targetsConfigured; } @@ -76,7 +65,8 @@ public long getTimeInMs() { return timeInMs; } - public int getActionsConstructed() { + /** Returns the actions constructed during this analysis. */ + public TotalAndConfiguredTargetOnlyMetric getActionsConstructed() { return actionsConstructed; } 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 09e460880a420f..561b98a9598bb1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -481,6 +481,7 @@ java_library( srcs = ["AnalysisPhaseCompleteEvent.java"], deps = [ ":configured_target", + "//src/main/java/com/google/devtools/build/lib/actions:total_and_configured_target_only_metric", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//third_party:guava", ], @@ -633,7 +634,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:target_pattern_phase_value", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/skyframe", - "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/protobuf:failure_details_java_proto", "//third_party:flogger", "//third_party:guava", 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 f5df8ce94da515..239ae1e94a9123 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 @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.analysis; -import static java.util.stream.Collectors.toSet; - import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Supplier; @@ -37,6 +35,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.PackageRoots; +import com.google.devtools.build.lib.actions.TotalAndConfiguredTargetOnlyMetric; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.BuildOptions; @@ -86,7 +85,6 @@ import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.RegexFilter; -import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.WalkableGraph; import java.util.ArrayList; import java.util.Collection; @@ -172,23 +170,13 @@ public BuildView(BlazeDirectories directories, this.skyframeBuildView = skyframeExecutor.getSkyframeBuildView(); } - /** - * Returns two numbers: number of analyzed and number of loaded targets. - * - *

The first number: configured targets freshly evaluated in the last analysis run. - * - *

The second number: targets (not configured targets) loaded in the last analysis run. - */ - public Pair getTargetsConfiguredAndLoaded() { - ImmutableSet keys = skyframeBuildView.getEvaluatedTargetKeys(); - int targetsConfigured = keys.size(); - int targetsLoaded = - keys.stream().map(key -> ((ConfiguredTargetKey) key).getLabel()).collect(toSet()).size(); - return Pair.of(targetsConfigured, targetsLoaded); + /** Returns the number of analyzed targets/aspects. */ + public TotalAndConfiguredTargetOnlyMetric getEvaluatedCounts() { + return skyframeBuildView.getEvaluatedCounts(); } - public int getActionsConstructed() { - return skyframeBuildView.getEvaluatedActionCount(); + public TotalAndConfiguredTargetOnlyMetric getEvaluatedActionsCounts() { + return skyframeBuildView.getEvaluatedActionCounts(); } public PackageManagerStatistics getAndClearPkgManagerStatistics() { @@ -430,7 +418,7 @@ public AnalysisResult update( viewOptions.strictConflictChecks); setArtifactRoots(skyframeAnalysisResult.getPackageRoots()); } finally { - skyframeBuildView.clearInvalidatedConfiguredTargets(); + skyframeBuildView.clearInvalidatedActionLookupKeys(); } TopLevelConstraintSemantics topLevelConstraintSemantics = 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 efb458a75ab8bc..2e00138514b608 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 @@ -745,14 +745,21 @@ message BuildFinished { message BuildMetrics { message ActionSummary { - // The total number of actions created and registered during the build. This - // includes unused actions that were constructed but not executed during - // this build. It does not include actions that were created on prior builds - // that are still valid, even if those actions had to be re-executed on this - // build. For the total number of actions that would be created if this - // invocation were "clean", see BuildGraphMetrics below. + // The total number of actions created and registered during the build, + // including both aspects and configured targets. This metric includes + // unused actions that were constructed but not executed during this build. + // It does not include actions that were created on prior builds that are + // still valid, even if those actions had to be re-executed on this build. + // For the total number of actions that would be created if this invocation + // were "clean", see BuildGraphMetrics below. int64 actions_created = 1; + // The total number of actions created this build just by configured + // targets. Used mainly to allow consumers of actions_created, which used to + // not include aspects' actions, to normalize across the Blaze release that + // switched actions_created to include all created actions. + int64 actions_created_not_including_aspects = 3; + // The total number of actions executed during the build. This includes any // remote cache hits, but excludes local action cache hits. int64 actions_executed = 2; @@ -772,15 +779,23 @@ message BuildMetrics { MemoryMetrics memory_metrics = 2; message TargetMetrics { - // Number of targets loaded during this build. Does not include targets that - // were loaded on prior builds on this server and were cached. + // DEPRECATED + // No longer populated. It never measured what it was supposed to (targets + // loaded): it counted targets that were analyzed even if the underlying + // package had not changed. + // TODO(janakr): rename and remove. int64 targets_loaded = 1; - // Number of targets configured during this build. This can be greater than - // targets_loaded if the same target is configured multiple times. Does not - // include targets that were configured on prior builds on this server and + // Number of targets/aspects configured during this build. Does not include + // targets/aspects that were configured on prior builds on this server and // were cached. See BuildGraphMetrics below if you need that. int64 targets_configured = 2; + + // Number of configured targets analyzed during this build. Does not include + // aspects. Used mainly to allow consumers of targets_configured, which used + // to not include aspects, to normalize across the Blaze release that + // switched targets_configured to include aspects. + int64 targets_configured_not_including_aspects = 3; } TargetMetrics target_metrics = 3; @@ -848,10 +863,18 @@ message BuildMetrics { // that were analyzed on a prior build and are still valid. May not be // populated if analysis phase was fully cached. int32 action_lookup_value_count = 1; + // How many configured targets alone were in this build: always at most + // action_lookup_value_count. Useful mainly for historical comparisons to + // TargetMetrics.targets_configured, which used to not count aspects. + int32 action_lookup_value_count_not_including_aspects = 5; // How many actions belonged to the configured targets/aspects above. It may // not be necessary to execute all of these actions to build the requested // targets. May not be populated if analysis phase was fully cached. int32 action_count = 2; + // How many actions belonged to configured targets: always at most + // action_count. Useful mainly for historical comparisons to + // ActionMetrics.actions_created, which used to not count aspects' actions. + int32 action_count_not_including_aspects = 6; // How many artifacts are outputs of the above actions. May not be populated // if analysis phase was fully cached. int32 output_artifact_count = 3; diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java index 6270462693108d..0837b4faa5f88a 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java @@ -51,7 +51,6 @@ import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.DetailedExitCode; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.RegexFilter; import com.google.devtools.common.options.OptionsParsingException; import java.util.Collection; @@ -228,18 +227,15 @@ private static AnalysisResult runAnalysisPhase( env.getReporter(), env.getEventBus()); - Pair tcal = view.getTargetsConfiguredAndLoaded(); - // TODO(bazel-team): Merge these into one event. env.getEventBus() .post( new AnalysisPhaseCompleteEvent( analysisResult.getTargetsToBuild(), - /* targetsLoaded */ tcal.second.intValue(), - /* targetsConfigured */ tcal.first.intValue(), + view.getEvaluatedCounts(), + view.getEvaluatedActionsCounts(), timer.stop().elapsed(TimeUnit.MILLISECONDS), view.getAndClearPkgManagerStatistics(), - view.getActionsConstructed(), env.getSkyframeExecutor().wasAnalysisCacheDiscardedAndResetBit())); ImmutableSet configurationKeys = Stream.concat( diff --git a/src/main/java/com/google/devtools/build/lib/metrics/BUILD b/src/main/java/com/google/devtools/build/lib/metrics/BUILD index 8f3f65d1665a6a..45af86c94365b5 100644 --- a/src/main/java/com/google/devtools/build/lib/metrics/BUILD +++ b/src/main/java/com/google/devtools/build/lib/metrics/BUILD @@ -34,6 +34,7 @@ java_library( ":memory-use-recorder", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:analysis_graph_stats_event", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_complete_event", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_started_event", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", 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 11db50d79f8a22..06f634d1b23a4d 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 @@ -17,6 +17,7 @@ import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.actions.ActionCompletionEvent; import com.google.devtools.build.lib.actions.AnalysisGraphStatsEvent; +import com.google.devtools.build.lib.actions.TotalAndConfiguredTargetOnlyMetric; import com.google.devtools.build.lib.analysis.AnalysisPhaseCompleteEvent; import com.google.devtools.build.lib.analysis.AnalysisPhaseStartedEvent; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics; @@ -84,10 +85,14 @@ public synchronized void logAnalysisStartingEvent(AnalysisPhaseStartedEvent even @SuppressWarnings("unused") @Subscribe public void onAnalysisPhaseComplete(AnalysisPhaseCompleteEvent event) { - actionSummary.setActionsCreated(event.getActionsConstructed()); + TotalAndConfiguredTargetOnlyMetric actionsConstructed = event.getActionsConstructed(); + actionSummary + .setActionsCreated(actionsConstructed.total()) + .setActionsCreatedNotIncludingAspects(actionsConstructed.configuredTargetsOnly()); + TotalAndConfiguredTargetOnlyMetric targetsConfigured = event.getTargetsConfigured(); targetMetrics - .setTargetsLoaded(event.getTargetsLoaded()) - .setTargetsConfigured(event.getTargetsConfigured()); + .setTargetsConfigured(targetsConfigured.total()) + .setTargetsConfiguredNotIncludingAspects(targetsConfigured.configuredTargetsOnly()); packageMetrics.setPackagesLoaded(event.getPkgManagerStats().getPackagesLoaded()); timingMetrics.setAnalysisPhaseTimeInMs(event.getTimeInMs()); } @@ -95,9 +100,14 @@ public void onAnalysisPhaseComplete(AnalysisPhaseCompleteEvent event) { @SuppressWarnings("unused") @Subscribe public synchronized void logAnalysisGraphStats(AnalysisGraphStatsEvent event) { + TotalAndConfiguredTargetOnlyMetric actionLookupValueCount = event.getActionLookupValueCount(); + TotalAndConfiguredTargetOnlyMetric actionCount = event.getActionCount(); buildGraphMetrics - .setActionLookupValueCount(event.getActionLookupValueCount()) - .setActionCount(event.getActionCount()) + .setActionLookupValueCount(actionLookupValueCount.total()) + .setActionLookupValueCountNotIncludingAspects( + actionLookupValueCount.configuredTargetsOnly()) + .setActionCount(actionCount.total()) + .setActionCountNotIncludingAspects(actionCount.configuredTargetsOnly()) .setOutputArtifactCount(event.getOutputArtifactCount()); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java index 801a2bcdbeeff0..15fb8427bf906d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java @@ -29,6 +29,8 @@ import com.google.devtools.build.lib.actions.MapBasedActionGraph; import com.google.devtools.build.lib.actions.MutableActionGraph; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; +import com.google.devtools.build.lib.actions.TotalAndConfiguredTargetOnlyMetric; +import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; @@ -72,7 +74,7 @@ static ActionConflictsAndStats findAndStoreArtifactConflicts( MapBasedActionGraph actionGraph = new MapBasedActionGraph(actionKeyContext); ConcurrentNavigableMap artifactPathMap = new ConcurrentSkipListMap<>(Actions.comparatorForPrefixConflicts()); - Pair counts = + Pair counts = constructActionGraphAndPathMap( actionGraph, artifactPathMap, actionLookupValues, temporaryBadActionMap); @@ -95,23 +97,30 @@ static ActionConflictsAndStats findAndStoreArtifactConflicts( * PathFragment}s to their respective {@link Artifact}s. We do this in a threadpool to save around * 1.5 seconds on a mid-sized build versus a single-threaded operation. */ - private static Pair constructActionGraphAndPathMap( - MutableActionGraph actionGraph, - ConcurrentNavigableMap artifactPathMap, - Iterable values, - ConcurrentMap badActionMap) - throws InterruptedException { + private static Pair + constructActionGraphAndPathMap( + MutableActionGraph actionGraph, + ConcurrentNavigableMap artifactPathMap, + Iterable values, + ConcurrentMap badActionMap) + throws InterruptedException { // Action graph construction is CPU-bound. int numJobs = Runtime.getRuntime().availableProcessors(); // No great reason for expecting 5000 action lookup values, but not worth counting size of // values. Sharder actionShards = new Sharder<>(numJobs, 5000); int actionLookupValueCount = 0; + int configuredTargetValueCount = 0; int actionCount = 0; + int configuredTargetActionCount = 0; for (ActionLookupValue value : values) { actionShards.add(value); actionLookupValueCount++; actionCount += value.getNumActions(); + if (value instanceof ConfiguredTargetValue) { + configuredTargetValueCount++; + configuredTargetActionCount += value.getNumActions(); + } } ThrowableRecordingRunnableWrapper wrapper = @@ -131,7 +140,10 @@ private static Pair constructActionGraphAndPathMap( if (interrupted) { throw new InterruptedException(); } - return Pair.of(actionLookupValueCount, actionCount); + return Pair.of( + TotalAndConfiguredTargetOnlyMetric.create( + actionLookupValueCount, configuredTargetValueCount), + TotalAndConfiguredTargetOnlyMetric.create(actionCount, configuredTargetActionCount)); } private static Runnable actionRegistration( @@ -206,8 +218,8 @@ abstract static class ActionConflictsAndStats { private static ActionConflictsAndStats create( ImmutableMap conflicts, - int actionValueCount, - int actionCount, + TotalAndConfiguredTargetOnlyMetric actionValueCount, + TotalAndConfiguredTargetOnlyMetric actionCount, int artifactCount) { return new AutoValue_ArtifactConflictFinder_ActionConflictsAndStats( conflicts, new AnalysisGraphStatsEvent(actionValueCount, actionCount, artifactCount)); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index be2e71c233fd05..3b42cd4ad35d3d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -765,7 +765,9 @@ java_library( deps = [ ":precomputed_value", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:analysis_graph_stats_event", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index 6d9a29e8354ce5..1161654b5c8435 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -15,7 +15,6 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.devtools.build.skyframe.EvaluationProgressReceiver.EvaluationState.BUILT; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; @@ -37,6 +36,7 @@ import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.PackageRoots; +import com.google.devtools.build.lib.actions.TotalAndConfiguredTargetOnlyMetric; import com.google.devtools.build.lib.analysis.AnalysisFailureEvent; import com.google.devtools.build.lib.analysis.AspectValue; import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment; @@ -123,17 +123,17 @@ public final class SkyframeBuildView { private final ActionKeyContext actionKeyContext; private boolean enableAnalysis = false; - // This hack allows us to see when a configured target has been invalidated, and thus when the set - // of artifact conflicts needs to be recomputed (whenever a configured target has been invalidated - // or newly evaluated). - private final ConfiguredTargetValueProgressReceiver progressReceiver = - new ConfiguredTargetValueProgressReceiver(); + // This hack allows us to see when an action lookup node has been invalidated, and thus when the + // set of artifact conflicts needs to be recomputed (whenever an action lookup node has been + // invalidated or newly evaluated). + private final ActionLookupValueProgressReceiver progressReceiver = + new ActionLookupValueProgressReceiver(); // Used to see if checks of graph consistency need to be done after analysis. - private volatile boolean someConfiguredTargetEvaluated = false; + private volatile boolean someActionLookupValueEvaluated = false; - // We keep the set of invalidated configuration target keys so that we can know if something - // has been invalidated after graph pruning has been executed. - private Set dirtiedConfiguredTargetKeys = Sets.newConcurrentHashSet(); + // We keep the set of invalidated action lookup nodes so that we can know if something has been + // invalidated after graph pruning has been executed. + private Set dirtiedActionLookupKeys = Sets.newConcurrentHashSet(); private final ConfiguredRuleClassProvider ruleClassProvider; @@ -171,16 +171,19 @@ public void resetProgressReceiver() { progressReceiver.reset(); } - public ImmutableSet getEvaluatedTargetKeys() { - return ImmutableSet.copyOf(progressReceiver.evaluatedConfiguredTargets); + public TotalAndConfiguredTargetOnlyMetric getEvaluatedCounts() { + return TotalAndConfiguredTargetOnlyMetric.create( + progressReceiver.actionLookupValueCount.get(), + progressReceiver.configuredTargetCount.get()); } ConfiguredTargetFactory getConfiguredTargetFactory() { return factory; } - public int getEvaluatedActionCount() { - return progressReceiver.evaluatedActionCount.get(); + public TotalAndConfiguredTargetOnlyMetric getEvaluatedActionCounts() { + return TotalAndConfiguredTargetOnlyMetric.create( + progressReceiver.actionCount.get(), progressReceiver.configuredTargetActionCount.get()); } /** @@ -450,7 +453,7 @@ public SkyframeAnalysisResult configureTargets( actionKeyContext); eventBus.post(conflictsAndStats.getStats()); actionConflicts = conflictsAndStats.getConflicts(); - someConfiguredTargetEvaluated = false; + someActionLookupValueEvaluated = false; } } foundActionConflict = !actionConflicts.isEmpty(); @@ -556,13 +559,13 @@ public SkyframeAnalysisResult configureTargets( } private boolean shouldCheckForConflicts(ImmutableSet newKeys) { - if (someConfiguredTargetEvaluated) { + if (someActionLookupValueEvaluated) { // A top-level target was added and may introduce a conflict, or a top-level target was // recomputed and may introduce or resolve a conflict. return true; } - if (!dirtiedConfiguredTargetKeys.isEmpty()) { + if (!dirtiedActionLookupKeys.isEmpty()) { // No target was (re)computed but at least one was dirtied. // Example: (//:x //foo:y) are built, and in conflict (//:x creates foo/C and //foo:y // creates C). Then y is removed from foo/BUILD and only //:x is built, so //foo:y is @@ -1035,9 +1038,9 @@ EvaluationProgressReceiver getProgressReceiver() { return progressReceiver; } - /** Clear the invalidated configured targets detected during loading and analysis phases. */ - public void clearInvalidatedConfiguredTargets() { - dirtiedConfiguredTargetKeys = Sets.newConcurrentHashSet(); + /** Clear the invalidated action lookup nodes detected during loading and analysis phases. */ + public void clearInvalidatedActionLookupKeys() { + dirtiedActionLookupKeys = Sets.newConcurrentHashSet(); } /** @@ -1054,20 +1057,21 @@ public ActionKeyContext getActionKeyContext() { return skyframeExecutor.getActionKeyContext(); } - private final class ConfiguredTargetValueProgressReceiver + private final class ActionLookupValueProgressReceiver extends EvaluationProgressReceiver.NullEvaluationProgressReceiver { - private final Set evaluatedConfiguredTargets = Sets.newConcurrentHashSet(); - private final AtomicInteger evaluatedActionCount = new AtomicInteger(); + private final AtomicInteger actionLookupValueCount = new AtomicInteger(); + private final AtomicInteger actionCount = new AtomicInteger(); + private final AtomicInteger configuredTargetCount = new AtomicInteger(); + private final AtomicInteger configuredTargetActionCount = new AtomicInteger(); @Override public void invalidated(SkyKey skyKey, InvalidationState state) { - if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET) - && state != InvalidationState.DELETED) { + if (skyKey instanceof ActionLookupKey && state != InvalidationState.DELETED) { // If the value was just dirtied and not deleted, then it may not be truly invalid, since // it may later get re-validated. Therefore adding the key to dirtiedConfiguredTargetKeys // is provisional--if the key is later evaluated and the value found to be clean, then we // remove it from the set. - dirtiedConfiguredTargetKeys.add(skyKey); + dirtiedActionLookupKeys.add((ActionLookupKey) skyKey); } } @@ -1077,34 +1081,42 @@ public void evaluated( @Nullable SkyValue value, Supplier evaluationSuccessState, EvaluationState state) { - if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) { - switch (state) { - case BUILT: - if (evaluationSuccessState.get().succeeded()) { - evaluatedConfiguredTargets.add(skyKey); - // During multithreaded operation, this is only set to true, so no concurrency issues. - someConfiguredTargetEvaluated = true; + // We tolerate any action lookup keys here, although we only expect configured targets, + // aspects, and the workspace status value. + if (!(skyKey instanceof ActionLookupKey)) { + return; + } + switch (state) { + case BUILT: + boolean isConfiguredTarget = skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET); + if (evaluationSuccessState.get().succeeded()) { + actionLookupValueCount.incrementAndGet(); + if (isConfiguredTarget) { + configuredTargetCount.incrementAndGet(); } - if (value instanceof ConfiguredTargetValue) { - evaluatedActionCount.addAndGet(((ConfiguredTargetValue) value).getNumActions()); + // During multithreaded operation, this is only set to true, so no concurrency issues. + someActionLookupValueEvaluated = true; + } + if (value instanceof ActionLookupValue) { + int numActions = ((ActionLookupValue) value).getNumActions(); + actionCount.addAndGet(numActions); + if (isConfiguredTarget) { + configuredTargetActionCount.addAndGet(numActions); } - break; - case CLEAN: - // If the configured target value did not need to be rebuilt, then it wasn't truly - // invalid. - dirtiedConfiguredTargetKeys.remove(skyKey); - break; - } - } else if (skyKey.functionName().equals(SkyFunctions.ASPECT) - && state == BUILT - && value instanceof AspectValue) { - evaluatedActionCount.addAndGet(((AspectValue) value).getNumActions()); + } + break; + case CLEAN: + // If the action lookup value did not need to be rebuilt, then it wasn't truly invalid. + dirtiedActionLookupKeys.remove(skyKey); + break; } } public void reset() { - evaluatedConfiguredTargets.clear(); - evaluatedActionCount.set(0); + actionLookupValueCount.set(0); + actionCount.set(0); + configuredTargetCount.set(0); + configuredTargetActionCount.set(0); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 7f3d7494b1516c..a275810bf5b908 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -872,7 +872,7 @@ public void handleAnalysisInvalidatingChange() { logger.atInfo().log("Dropping configured target data"); analysisCacheDiscarded = true; clearTrimmingCache(); - skyframeBuildView.clearInvalidatedConfiguredTargets(); + skyframeBuildView.clearInvalidatedActionLookupKeys(); skyframeBuildView.clearLegacyData(); ArtifactNestedSetFunction.getInstance().resetArtifactNestedSetFunctionMaps(); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java index f1a4d1209d037f..d2a97a57a13b1d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java @@ -920,4 +920,77 @@ public void sameConfiguredAttributeOnAspectAndRule() throws Exception { setRulesAndAspectsAvailableInTests(ImmutableList.of(), ImmutableList.of()); getConfiguredTarget("//a:r"); } + + @Test + public void topLevelConflictDetected() throws Exception { + String bzlFileTemplate = + String.join( + "\n", + "def _aspect1_impl(target, ctx):", + " outfile = ctx.actions.declare_file('aspect.out')", + " ctx.actions.run_shell(", + " outputs = [outfile],", + " progress_message = 'Action for aspect 1',", + " command = 'echo \"1\" > ' + outfile.path,", + " )", + " return [OutputGroupInfo(files = [outfile])]", + "def _aspect2_impl(target, ctx):", + " outfile = ctx.actions.declare_file('aspect.out')", + " ctx.actions.run_shell(", + " outputs = [outfile],", + " progress_message = 'Action for aspect 2',", + " command = 'echo \"%s\" > ' + outfile.path,", + " )", + " return [OutputGroupInfo(files = [outfile])]", + "aspect1 = aspect(implementation = _aspect1_impl)", + "aspect2 = aspect(implementation = _aspect2_impl)"); + scratch.file("foo/aspect.bzl", String.format(bzlFileTemplate, "2")); + scratch.file("foo/BUILD", "sh_library(name = 'foo', srcs = ['foo.sh'])"); + // Expect errors. + reporter.removeHandler(failFastHandler); + ViewCreationFailedException exception = + assertThrows( + ViewCreationFailedException.class, + () -> + update( + new EventBus(), + defaultFlags(), + ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"), + "//foo:foo")); + assertThat(exception) + .hasMessageThat() + .contains( + "ConflictException: for foo/aspect.out, previous action: action 'Action for aspect 1'," + + " attempted action: action 'Action for aspect 2'"); + + // Fix bzl file so actions are shared: analysis should succeed now. + scratch.overwriteFile("foo/aspect.bzl", String.format(bzlFileTemplate, "1")); + reporter.addHandler(failFastHandler); + AnalysisResult result = + update( + new EventBus(), + defaultFlags(), + ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"), + "//foo:foo"); + assertThat(result.getAspectsMap()).hasSize(2); + + // Break bzl file again: we should notice. + scratch.overwriteFile("foo/aspect.bzl", String.format(bzlFileTemplate, "2")); + // Expect errors. + reporter.removeHandler(failFastHandler); + exception = + assertThrows( + ViewCreationFailedException.class, + () -> + update( + new EventBus(), + defaultFlags(), + ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"), + "//foo:foo")); + assertThat(exception) + .hasMessageThat() + .contains( + "ConflictException: for foo/aspect.out, previous action: action 'Action for aspect 1'," + + " attempted action: action 'Action for aspect 2'"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD index 88c941bd07bff1..555f696342ea6f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD @@ -38,6 +38,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", + "//src/main/java/com/google/devtools/build/lib/actions:artifact_owner", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", @@ -69,7 +70,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", - "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_provider", "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/per_label_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/run_under", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java index 95d72e7d1fbacc..f52a4ee229de45 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java @@ -22,7 +22,9 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMultiset; +import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -45,7 +47,6 @@ import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; -import com.google.devtools.build.skyframe.SkyKey; import java.util.Arrays; import java.util.LinkedHashSet; import java.util.Map; @@ -138,11 +139,11 @@ public void setUp() throws Exception { } private void assertNumberOfConfigurationsOfTargets( - Set keys, Map targetsWithCounts) { + Set keys, Map targetsWithCounts) { ImmutableMultiset