From 2a3ab5c45b42321cf1495be7e294e5ec01298ca1 Mon Sep 17 00:00:00 2001
From: Googler <mschaller@google.com>
Date: Wed, 22 Mar 2023 07:25:08 -0700
Subject: [PATCH] Enable heuristically dropping GENQUERY_SCOPE nodes

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

PiperOrigin-RevId: 518561737
Change-Id: Ic76aab06be17674ed4e3bb574299b5abceb82d70
---
 .../devtools/build/lib/rules/genquery/BUILD   | 17 ++++++++----
 .../GenQueryDirectPackageProviderFactory.java |  2 +-
 .../google/devtools/build/lib/skyframe/BUILD  |  3 ++-
 .../NodeDroppingInconsistencyReceiver.java    | 17 +++++++-----
 .../build/lib/skyframe/SkyframeExecutor.java  | 26 +++++++++++++++++++
 5 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/BUILD b/src/main/java/com/google/devtools/build/lib/rules/genquery/BUILD
index 1c38ded65c24aa..a3fc23535a0e61 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/BUILD
@@ -17,11 +17,11 @@ 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",
@@ -29,10 +29,7 @@ java_library(
         "//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",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java
index 835c6cc3e6b993..4bc2cde2a9053a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java
+++ b/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) {
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 76f8bbe200e78a..90aa99db450ca2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/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",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java
index 612e3a5c855468..ae2a6ef669483c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java
+++ b/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;
   }
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 bcdbf1f33ff539..5f6d0131a90b13 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
@@ -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;
@@ -3117,6 +3119,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);
         }
       }
 
@@ -3173,6 +3178,27 @@ 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) {
+          memoizingEvaluator.getInMemoryGraph().remove(key);
+          return;
+        }
+      }
+    }
+
     private void recursivelyRemoveGlobFromGraph(GlobDescriptor root) {
       memoizingEvaluator.getInMemoryGraph().remove(root);
       ImmutableList<GlobDescriptor> adjacentDeps = globDeps.remove(root);