Enable heuristically dropping GENQUERY_SCOPE nodes

These are often not used after their parent genquery's configured target
value is evaluated.

PiperOrigin-RevId: 520039728
Change-Id: I96de7bf5a6f4b5b3e229dff249573f1b5152411f

src/main/java/com/google/devtools/build/lib/rules/genquery/BUILD

@@ -17,22 +17,19 @@ java_library(
     name = "genquery",
     srcs = [
         "GenQuery.java",
-        "GenQueryConfiguration.java",
         "GenQueryOutputStream.java",
         "GenQueryRule.java",
     ],
     deps = [
+        ":genquery_configuration",
         ":genquery_package_providers",
         "//src/main/java/com/google/devtools/build/lib:keep-going-option",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/abstract_file_write_action",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/deterministic_writer",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
-        "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
-        "//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:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
@@ -64,6 +61,17 @@ java_library(
     ],
 )
 
+java_library(
+    name = "genquery_configuration",
+    srcs = ["GenQueryConfiguration.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
+        "//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/common/options",
+    ],
+)
+
 java_library(
     name = "genquery_package_providers",
     srcs = [
@@ -76,7 +84,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/bugreport",
         "//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/concurrent",
         "//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/pkgcache",

src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java

@@ -72,7 +72,7 @@ public class GenQueryDirectPackageProviderFactory implements GenQueryPackageProv
    * rules sharing the same scope will require only one scope traversal to occur.
    */
   @AutoCodec
-  static class Key extends AbstractSkyKey.WithCachedHashCode<ImmutableList<Label>> {
+  public static class Key extends AbstractSkyKey.WithCachedHashCode<ImmutableList<Label>> {
     private static final SkyKeyInterner<Key> interner = SkyKey.newInterner();
 
     private Key(ImmutableList<Label> arg) {

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -148,7 +148,6 @@ java_library(
         ":ignored_package_prefixes_function",
         ":ignored_package_prefixes_value",
         ":incremental_artifact_conflict_finder",
-        ":incremental_package_roots",
         ":loading_phase_started_event",
         ":local_repository_lookup_value",
         ":map_as_package_roots",
@@ -331,6 +330,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
         "//src/main/java/com/google/devtools/build/lib/rules:repository/resolved_file_value",
         "//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_file_helper",
+        "//src/main/java/com/google/devtools/build/lib/rules/genquery:genquery_configuration",
         "//src/main/java/com/google/devtools/build/lib/rules/genquery:genquery_package_providers",
         "//src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2:actiongraph_v2",
         "//src/main/java/com/google/devtools/build/lib/skyframe/rewinding",
@@ -3035,6 +3035,7 @@ java_library(
     deps = [
         ":sky_functions",
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
+        "//src/main/java/com/google/devtools/build/lib/rules/genquery:genquery_package_providers",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/skyframe",
         "//src/main/java/com/google/devtools/build/skyframe:graph_inconsistency_java_proto",

src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java

@@ -15,7 +15,9 @@
 
 import static com.google.common.base.Preconditions.checkState;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.FileValue;
+import com.google.devtools.build.lib.rules.genquery.GenQueryDirectPackageProviderFactory;
 import com.google.devtools.build.lib.vfs.FileStateKey;
 import com.google.devtools.build.skyframe.GraphInconsistencyReceiver;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -36,6 +38,12 @@
  */
 public class NodeDroppingInconsistencyReceiver implements GraphInconsistencyReceiver {
 
+  private static final ImmutableMap<SkyFunctionName, SkyFunctionName> EXPECTED_MISSING_CHILDREN =
+      ImmutableMap.of(
+          FileValue.FILE, FileStateKey.FILE_STATE,
+          SkyFunctions.DIRECTORY_LISTING, SkyFunctions.DIRECTORY_LISTING_STATE,
+          SkyFunctions.CONFIGURED_TARGET, GenQueryDirectPackageProviderFactory.GENQUERY_SCOPE);
+
   @Override
   public void noteInconsistencyAndMaybeThrow(
       SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
@@ -53,9 +61,8 @@ public void noteInconsistencyAndMaybeThrow(
    */
   public static boolean isExpectedInconsistency(
       SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
-    boolean isFileKey = key.functionName().equals(FileValue.FILE);
-    boolean isDirectoryListingKey = key.functionName().equals(SkyFunctions.DIRECTORY_LISTING);
-    if (!isFileKey && !isDirectoryListingKey) {
+    SkyFunctionName expectedMissingChildType = EXPECTED_MISSING_CHILDREN.get(key.functionName());
+    if (expectedMissingChildType == null) {
       return false;
     }
     if (inconsistency == Inconsistency.RESET_REQUESTED) {
@@ -65,9 +72,7 @@ public static boolean isExpectedInconsistency(
         || inconsistency == Inconsistency.BUILDING_PARENT_FOUND_UNDONE_CHILD) {
       // For already declared child missing inconsistency, key is the parent while `otherKeys`
       // are the children (dependency nodes).
-      SkyFunctionName expectedStateKeyType =
-          isFileKey ? FileStateKey.FILE_STATE : SkyFunctions.DIRECTORY_LISTING_STATE;
-      return otherKeys.stream().allMatch(SkyFunctionName.functionIs(expectedStateKeyType));
+      return otherKeys.stream().allMatch(SkyFunctionName.functionIs(expectedMissingChildType));
     }
     return false;
   }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -99,6 +99,7 @@
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.NullTransition;
 import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkBuildSettingsDetailsValue;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition;
@@ -157,6 +158,7 @@
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
 import com.google.devtools.build.lib.repository.ExternalPackageHelper;
+import com.google.devtools.build.lib.rules.genquery.GenQueryConfiguration.GenQueryOptions;
 import com.google.devtools.build.lib.rules.genquery.GenQueryDirectPackageProviderFactory;
 import com.google.devtools.build.lib.rules.repository.ResolvedFileFunction;
 import com.google.devtools.build.lib.rules.repository.ResolvedHashesFunction;
@@ -3124,6 +3126,9 @@ public void evaluated(
               argument);
           SkyKey directoryListingStateKey = DirectoryListingStateValue.key((RootedPath) argument);
           memoizingEvaluator.getInMemoryGraph().remove(directoryListingStateKey);
+        } else if (directDeps != null
+            && skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
+          maybeDropGenQueryDep(newValue, directDeps);
         }
       }
 
@@ -3180,6 +3185,33 @@ public void evaluated(
       }
     }
 
+    private void maybeDropGenQueryDep(SkyValue newValue, GroupedDeps directDeps) {
+      if (!(newValue instanceof RuleConfiguredTargetValue)) {
+        return;
+      }
+      var t = (RuleConfiguredTarget) ((RuleConfiguredTargetValue) newValue).getConfiguredTarget();
+      if (!t.getRuleClassString().equals("genquery")) {
+        return;
+      }
+      BuildOptions options = t.getConfigurationKey().getOptions();
+      if (!options.contains(GenQueryOptions.class)
+          || !options.get(GenQueryOptions.class).skipTtvs) {
+        return;
+      }
+      for (SkyKey key : directDeps.getAllElementsAsIterable()) {
+        if (key instanceof GenQueryDirectPackageProviderFactory.Key) {
+          // The following call can occur several times for the same GENQUERY_SCOPE key in a single
+          // Skyframe evaluation, because multiple genquery configured targets may have deps on the
+          // same GENQUERY_SCOPE node. It is #removeIfDone and not merely #remove because not-done
+          // nodes cannot be removed from the graph, because they may own state which the Skyframe
+          // evaluation depends on for its completion, namely, the list of rdeps which must be
+          // signaled when the node finishes evaluation.
+          memoizingEvaluator.getInMemoryGraph().removeIfDone(key);
+          return;
+        }
+      }
+    }
+
     private void recursivelyRemoveGlobFromGraph(GlobDescriptor root) {
       memoizingEvaluator.getInMemoryGraph().remove(root);
       ImmutableList<GlobDescriptor> adjacentDeps = globDeps.remove(root);

src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java

@@ -454,13 +454,12 @@ private boolean handleKnownChildrenForDirtyNode(
     public void run() {
       SkyFunctionEnvironment env = null;
       try {
-        NodeEntry nodeEntry = checkNotNull(graph.get(null, Reason.EVALUATION, skyKey), skyKey);
-        if (nodeEntry.isDone()) {
+        NodeEntry nodeEntry = graph.get(null, Reason.EVALUATION, skyKey);
+        if (nodeEntry == null || nodeEntry.isDone() || !nodeEntry.isReadyToEvaluate()) {
           checkState(skyKey.supportsPartialReevaluation(), "%s %s", skyKey, nodeEntry);
           evaluatorContext.getProgressReceiver().removeFromInflight(skyKey);
           return;
         }
-        checkState(nodeEntry.isReadyToEvaluate(), "%s %s", skyKey, nodeEntry);
         try {
           evaluatorContext.getProgressReceiver().stateStarting(skyKey, NodeState.CHECK_DIRTY);
           if (maybeHandleDirtyNode(nodeEntry) == DirtyOutcome.ALREADY_PROCESSED) {

src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java

@@ -86,4 +86,9 @@ default int valuesSize() {
 
   /** Applies the given consumer to each node in the graph, potentially in parallel. */
   void parallelForEach(Consumer<InMemoryNodeEntry> consumer);
+
+  /**
+   * Removes the node entry associated with the given {@link SkyKey} from the graph if it is done.
+   */
+  void removeIfDone(SkyKey key);
 }

src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java

@@ -77,6 +77,19 @@ public void remove(SkyKey skyKey) {
     nodeMap.remove(skyKey);
   }
 
+  @Override
+  public void removeIfDone(SkyKey key) {
+    nodeMap.computeIfPresent(
+        key,
+        (k, e) -> {
+          if (e.isDone()) {
+            weakIntern(k);
+            return null;
+          }
+          return e;
+        });
+  }
+
   private void weakIntern(SkyKey skyKey) {
     if (!usePooledSkyKeyInterning) {
       return;

src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java

@@ -88,4 +88,9 @@ public SkyKey getOrWeakIntern(SkyKey key) {
   public void cleanupPool() {
     ((InMemoryGraph) delegate).cleanupPool();
   }
+
+  @Override
+  public void removeIfDone(SkyKey key) {
+    ((InMemoryGraph) delegate).removeIfDone(key);
+  }
 }

src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java

@@ -95,4 +95,9 @@ public SkyKey getOrWeakIntern(SkyKey key) {
   public void cleanupPool() {
     ((InMemoryGraph) delegate).cleanupPool();
   }
+
+  @Override
+  public void removeIfDone(SkyKey key) {
+    ((InMemoryGraph) delegate).removeIfDone(key);
+  }
 }