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

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;
   }
 

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"],

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);
+  }
+}

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<ConfiguredTarget> 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<? extends ConfiguredTarget> 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,21 +56,17 @@ public Collection<ConfiguredTarget> 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;
   }
 
   public long getTimeInMs() {
     return timeInMs;
   }
 
-  public int getActionsConstructed() {
+  /** Returns the actions constructed during this analysis. */
+  public TotalAndConfiguredTargetOnlyMetric getActionsConstructed() {
     return actionsConstructed;
   }
 

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",

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.
-   *
-   * <p>The first number: configured targets freshly evaluated in the last analysis run.
-   *
-   * <p>The second number: targets (not configured targets) loaded in the last analysis run.
-   */
-  public Pair<Integer, Integer> getTargetsConfiguredAndLoaded() {
-    ImmutableSet<SkyKey> 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 =

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;

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<Integer, Integer> 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<BuildConfigurationValue.Key> configurationKeys =
         Stream.concat(

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",