From 078c14cb13d84c6ce1f56dcb3dba3e975c3a2f52 Mon Sep 17 00:00:00 2001
From: Googler <shahan@google.com>
Date: Thu, 2 Mar 2023 08:33:54 -0800
Subject: [PATCH] Memoize
 IncompatibleTargetChecker.createDirectlyIncompatibleTarget.

This uses StateMachine to avoid recomputation on restart.

PiperOrigin-RevId: 513546261
Change-Id: Ice4245a72fe29ccd8dda040910460626d5041512
---
 .../google/devtools/build/lib/analysis/BUILD  |   3 +-
 .../IncompatibleTargetChecker.java            | 151 ++++++++++--------
 .../lib/skyframe/PrerequisiteProducer.java    |  61 ++++++-
 3 files changed, 142 insertions(+), 73 deletions(-)

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 def0888963071f..a6c1ab6b77d1bc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -2256,13 +2256,13 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
-        "//src/main/java/com/google/devtools/build/lib/analysis:dependency",
         "//src/main/java/com/google/devtools/build/lib/analysis:dependency_kind",
         "//src/main/java/com/google/devtools/build/lib/analysis:target_and_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
+        "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
@@ -2270,6 +2270,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:rule_configured_target_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",
         "//third_party:guava",
         "//third_party:jsr305",
     ],
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java
index 2f4bb040daf303..b22578cbfecc27 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis.constraints;
 
-import static com.google.common.base.Predicates.notNull;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
 import com.google.common.collect.ImmutableList;
@@ -22,7 +21,6 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
-import com.google.devtools.build.lib.analysis.Dependency;
 import com.google.devtools.build.lib.analysis.DependencyKind;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
@@ -44,6 +42,7 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
 import com.google.devtools.build.lib.packages.Package;
@@ -55,10 +54,10 @@
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import com.google.devtools.build.lib.skyframe.RuleConfiguredTargetValue;
 import com.google.devtools.build.lib.util.OrderedSetMultimap;
-import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyframeLookupResult;
-import java.util.List;
+import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.build.skyframe.state.StateMachine;
 import java.util.Optional;
+import java.util.function.Consumer;
 import javax.annotation.Nullable;
 
 /**
@@ -87,81 +86,105 @@ public class IncompatibleTargetChecker {
   /**
    * Creates an incompatible configured target if it is "directly incompatible".
    *
-   * <p>In other words, this function checks if a target is incompatible because of its
+   * <p>In other words, this state machine checks if a target is incompatible because of its
    * "target_compatible_with" attribute.
    *
-   * <p>This function returns a nullable {@code Optional} of a {@link RuleConfiguredTargetValue}.
-   * This provides three states of return values:
+   * <p>Outputs an {@code Optional} {@link RuleConfiguredTargetValue} as follows.
    *
    * <ul>
-   *   <li>{@code null}: A skyframe restart is required.
    *   <li>{@code Optional.empty()}: The target is not directly incompatible. Analysis can continue.
    *   <li>{@code !Optional.empty()}: The target is directly incompatible. Analysis should not
    *       continue.
    * </ul>
    */
-  @Nullable
-  public static Optional<RuleConfiguredTargetValue> createDirectlyIncompatibleTarget(
-      TargetAndConfiguration targetAndConfiguration,
-      ConfigConditions configConditions,
-      Environment env,
-      @Nullable PlatformInfo platformInfo,
-      NestedSetBuilder<Package> transitivePackages)
-      throws InterruptedException {
-    Target target = targetAndConfiguration.getTarget();
-    Rule rule = target.getAssociatedRule();
+  public static class IncompatibleTargetProducer implements StateMachine, Consumer<SkyValue> {
+    private final Target target;
+    @Nullable // Non-null when the target has an associated rule.
+    private final BuildConfigurationValue configuration;
+    private final ConfigConditions configConditions;
+    @Nullable private final PlatformInfo platformInfo;
+    @Nullable private final NestedSetBuilder<Package> transitivePackages;
 
-    if (rule == null || rule.getRuleClass().equals("toolchain") || platformInfo == null) {
-      return Optional.empty();
+    private final ResultSink sink;
+
+    private final ImmutableList.Builder<ConstraintValueInfo> invalidConstraintValuesBuilder =
+        new ImmutableList.Builder<>();
+
+    /** Sink for the output of this state machine. */
+    public interface ResultSink {
+      void accept(Optional<RuleConfiguredTargetValue> incompatibleTarget);
     }
 
-    // Retrieve the label list for the target_compatible_with attribute.
-    BuildConfigurationValue configuration = targetAndConfiguration.getConfiguration();
-    ConfiguredAttributeMapper attrs =
-        ConfiguredAttributeMapper.of(rule, configConditions.asProviders(), configuration);
-    if (!attrs.has("target_compatible_with", BuildType.LABEL_LIST)) {
-      return Optional.empty();
+    public IncompatibleTargetProducer(
+        Target target,
+        @Nullable BuildConfigurationValue configuration,
+        ConfigConditions configConditions,
+        @Nullable PlatformInfo platformInfo,
+        @Nullable NestedSetBuilder<Package> transitivePackages,
+        ResultSink sink) {
+      this.target = target;
+      this.configuration = configuration;
+      this.configConditions = configConditions;
+      this.platformInfo = platformInfo;
+      this.transitivePackages = transitivePackages;
+      this.sink = sink;
     }
 
-    // Resolve the constraint labels.
-    List<Label> labels = attrs.get("target_compatible_with", BuildType.LABEL_LIST);
-    ImmutableList<ConfiguredTargetKey> constraintKeys =
-        labels.stream()
-            .map(
-                label ->
-                    Dependency.builder()
-                        .setLabel(label)
-                        .setConfiguration(configuration)
-                        .build()
-                        .getConfiguredTargetKey())
-            .collect(toImmutableList());
-    SkyframeLookupResult constraintValues = env.getValuesAndExceptions(constraintKeys);
-    if (env.valuesMissing()) {
-      return null;
+    @Override
+    @Nullable
+    public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
+      Rule rule = target.getAssociatedRule();
+      if (rule == null || rule.getRuleClass().equals("toolchain") || platformInfo == null) {
+        sink.accept(Optional.empty());
+        return null;
+      }
+
+      // Retrieves the label list for the target_compatible_with attribute.
+      ConfiguredAttributeMapper attrs =
+          ConfiguredAttributeMapper.of(rule, configConditions.asProviders(), configuration);
+      if (!attrs.has("target_compatible_with", BuildType.LABEL_LIST)) {
+        sink.accept(Optional.empty());
+        return null;
+      }
+
+      // Resolves the constraint labels.
+      for (Label label : attrs.get("target_compatible_with", BuildType.LABEL_LIST)) {
+        tasks.lookUp(
+            ConfiguredTargetKey.builder().setLabel(label).setConfiguration(configuration).build(),
+            this);
+      }
+      return this::processResult;
     }
 
-    // Find the constraints that don't satisfy the target platform.
-    ImmutableList<ConstraintValueInfo> invalidConstraintValues =
-        constraintKeys.stream()
-            .map(key -> (ConfiguredTargetValue) constraintValues.get(key))
-            .filter(notNull())
-            .map(ctv -> PlatformProviderUtils.constraintValue(ctv.getConfiguredTarget()))
-            .filter(notNull())
-            .filter(cv -> !platformInfo.constraints().hasConstraintValue(cv))
-            .collect(toImmutableList());
-    if (invalidConstraintValues.isEmpty()) {
-      return Optional.empty();
+    @Override
+    public void accept(SkyValue value) {
+      var configuredTarget = ((ConfiguredTargetValue) value).getConfiguredTarget();
+      @Nullable ConstraintValueInfo info = PlatformProviderUtils.constraintValue(configuredTarget);
+      if (info == null || platformInfo.constraints().hasConstraintValue(info)) {
+        return;
+      }
+      invalidConstraintValuesBuilder.add(info);
     }
 
-    return Optional.of(
-        createIncompatibleRuleConfiguredTarget(
-            target,
-            configuration,
-            configConditions,
-            IncompatiblePlatformProvider.incompatibleDueToConstraints(
-                platformInfo.label(), invalidConstraintValues),
-            rule.getRuleClass(),
-            transitivePackages));
+    @Nullable
+    private StateMachine processResult(Tasks tasks, ExtendedEventHandler listener) {
+      var invalidConstraintValues = invalidConstraintValuesBuilder.build();
+      if (invalidConstraintValues.isEmpty()) {
+        sink.accept(Optional.empty());
+        return null;
+      }
+      sink.accept(
+          Optional.of(
+              createIncompatibleRuleConfiguredTarget(
+                  target,
+                  configuration,
+                  configConditions,
+                  IncompatiblePlatformProvider.incompatibleDueToConstraints(
+                      platformInfo.label(), invalidConstraintValues),
+                  target.getAssociatedRule().getRuleClass(),
+                  transitivePackages)));
+      return null;
+    }
   }
 
   /**
@@ -185,7 +208,7 @@ public static Optional<RuleConfiguredTargetValue> createIndirectlyIncompatibleTa
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> depValueMap,
       ConfigConditions configConditions,
       @Nullable PlatformInfo platformInfo,
-      NestedSetBuilder<Package> transitivePackages) {
+      @Nullable NestedSetBuilder<Package> transitivePackages) {
     Target target = targetAndConfiguration.getTarget();
     Rule rule = target.getAssociatedRule();
 
@@ -223,7 +246,7 @@ private static RuleConfiguredTargetValue createIncompatibleRuleConfiguredTarget(
       ConfigConditions configConditions,
       IncompatiblePlatformProvider incompatiblePlatformProvider,
       String ruleClassString,
-      NestedSetBuilder<Package> transitivePackages) {
+      @Nullable NestedSetBuilder<Package> transitivePackages) {
     // Create dummy instances of the necessary data for a configured target. None of this data will
     // actually be used because actions associated with incompatible targets must not be evaluated.
     NestedSet<Artifact> filesToBuild = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java
index 44d79b9bd9d0c7..a70e0cab5f7f2b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java
@@ -47,6 +47,7 @@
 import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.constraints.IncompatibleTargetChecker;
+import com.google.devtools.build.lib.analysis.constraints.IncompatibleTargetChecker.IncompatibleTargetProducer;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.causes.Cause;
@@ -79,6 +80,7 @@
 import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyframeLookupResult;
+import com.google.devtools.build.skyframe.state.Driver;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -113,9 +115,22 @@
  * <p>See {@link ConfiguredTargetFunction} for more review on analysis implementation.
  */
 public final class PrerequisiteProducer {
-  static class State implements SkyKeyComputeState {
+  static class State implements SkyKeyComputeState, IncompatibleTargetProducer.ResultSink {
     @Nullable TargetAndConfiguration targetAndConfiguration;
 
+    /**
+     * Drives the stateful computation of {@link #incompatibleTarget}.
+     *
+     * <p>Non-null only while the computation is in-flight.
+     */
+    @Nullable private Driver incompatibleTargetProducer;
+    /**
+     * If a value is present, it means the target was directly incompatible.
+     *
+     * <p>Non-null after the {@link #incompatibleTargetProducer} completes.
+     */
+    private Optional<RuleConfiguredTargetValue> incompatibleTarget;
+
     /** Null if not yet computed or if {@link #resolveConfigurationsResult} is non-null. */
     @Nullable private OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMapResult;
 
@@ -154,6 +169,11 @@ static class State implements SkyKeyComputeState {
      * thrown away.
      */
     @Nullable private StoredEventHandler storedEventHandlerFromResolveConfigurations;
+
+    @Override
+    public void accept(Optional<RuleConfiguredTargetValue> incompatibleTarget) {
+      this.incompatibleTarget = incompatibleTarget;
+    }
   }
 
   /**
@@ -341,15 +361,9 @@ public boolean evaluate(
                 getPrioritizedDetailedExitCode(causes)));
       }
 
-      Optional<RuleConfiguredTargetValue> incompatibleTarget =
-          IncompatibleTargetChecker.createDirectlyIncompatibleTarget(
-              targetAndConfiguration, configConditions, env, platformInfo, transitivePackages);
-      if (incompatibleTarget == null) {
+      if (!checkForIncompatibleTarget(env, state, transitivePackages)) {
         return false;
       }
-      if (incompatibleTarget.isPresent()) {
-        throw new IncompatibleTargetException(incompatibleTarget.get());
-      }
 
       // Calculate the dependencies of this target.
       depValueMap =
@@ -391,6 +405,37 @@ public boolean evaluate(
     return true;
   }
 
+  /**
+   * Checks if a target is incompatible because of its "target_compatible_with" attribute.
+   *
+   * @return false if a {@code Skyframe} restart is needed.
+   */
+  private boolean checkForIncompatibleTarget(
+      Environment env, State state, @Nullable NestedSetBuilder<Package> transitivePackages)
+      throws InterruptedException, IncompatibleTargetException {
+    if (state.incompatibleTarget == null) {
+      if (state.incompatibleTargetProducer == null) {
+        state.incompatibleTargetProducer =
+            new Driver(
+                new IncompatibleTargetProducer(
+                    targetAndConfiguration.getTarget(),
+                    targetAndConfiguration.getConfiguration(),
+                    configConditions,
+                    platformInfo,
+                    transitivePackages,
+                    state));
+      }
+      if (!state.incompatibleTargetProducer.drive(env, env.getListener())) {
+        return false;
+      }
+      state.incompatibleTargetProducer = null;
+      if (state.incompatibleTarget.isPresent()) {
+        throw new IncompatibleTargetException(state.incompatibleTarget.get());
+      }
+    }
+    return true;
+  }
+
   /**
    * Handles all exceptions that {@link #evaluate} may throw.
    *