Drop loading-phase values if --discard_analysis_cache is true and we're not keeping incremental state.

PiperOrigin-RevId: 151639711

8 files changed, 152 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 61deface5d..e48991b97e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -63,6 +63,7 @@ import com.google.devtools.build.skyframe.Differencer;
 import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
 import com.google.devtools.build.skyframe.Injectable;
 import com.google.devtools.build.skyframe.MemoizingEvaluator.EvaluatorSupplier;
+import com.google.devtools.build.skyframe.NodeEntry;
 import com.google.devtools.build.skyframe.RecordingDifferencer;
 import com.google.devtools.build.skyframe.SequentialBuildDriver;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -75,6 +76,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.EnumSet;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -595,15 +597,26 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
     recordingDiffer.invalidate(dirtyActionValues);
   }
 
+  private static ImmutableSet<SkyFunctionName> LOADING_TYPES =
+      ImmutableSet.of(
+          SkyFunctions.PACKAGE,
+          SkyFunctions.SKYLARK_IMPORTS_LOOKUP,
+          SkyFunctions.AST_FILE_LOOKUP,
+          SkyFunctions.GLOB);
+
   /**
    * Save memory by removing references to configured targets and aspects in Skyframe.
    *
-   * <p>These values must be recreated on subsequent builds. We do not clear the top-level target
-   * values, since their configured targets are needed for the target completion middleman values.
+   * <p>These nodes must be recreated on subsequent builds. We do not clear the top-level target
+   * nodes, since their configured targets are needed for the target completion middleman values.
+   *
+   * <p>The nodes are not deleted during this method call, because they are needed for the execution
+   * phase. Instead, their analysis-time data is cleared while preserving the generating action info
+   * needed for execution. The next build will delete the nodes (and recreate them if necessary).
    *
-   * <p>The values are not deleted during this method call, because they are needed for the
-   * execution phase. Instead, their data is cleared. The next build will delete the values (and
-   * recreate them if necessary).
+   * <p>If {@link #hasIncrementalState} is false, then also delete loading-phase nodes (as
+   * determined by {@link #LOADING_TYPES}) from the graph, since there will be no future builds to
+   * use them for.
    */
   private void discardAnalysisCache(
       Collection<ConfiguredTarget> topLevelTargets, Collection<AspectValue> topLevelAspects) {
@@ -611,16 +624,38 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
     topLevelAspects = ImmutableSet.copyOf(topLevelAspects);
     try (AutoProfiler p = AutoProfiler.logged("discarding analysis cache", LOG)) {
       lastAnalysisDiscarded = true;
-      for (Map.Entry<SkyKey, SkyValue> entry : memoizingEvaluator.getValues().entrySet()) {
-        SkyFunctionName functionName = entry.getKey().functionName();
+      Iterator<? extends Map.Entry<SkyKey, ? extends NodeEntry>> it =
+          memoizingEvaluator.getGraphMap().entrySet().iterator();
+      while (it.hasNext()) {
+        Map.Entry<SkyKey, ? extends NodeEntry> keyAndEntry = it.next();
+        NodeEntry entry = keyAndEntry.getValue();
+        if (entry == null || !entry.isDone()) {
+          continue;
+        }
+        SkyKey key = keyAndEntry.getKey();
+        SkyFunctionName functionName = key.functionName();
+        if (!hasIncrementalState() && LOADING_TYPES.contains(functionName)) {
+          it.remove();
+          continue;
+        }
         if (functionName.equals(SkyFunctions.CONFIGURED_TARGET)) {
-          ConfiguredTargetValue ctValue = (ConfiguredTargetValue) entry.getValue();
+          ConfiguredTargetValue ctValue;
+          try {
+            ctValue = (ConfiguredTargetValue) entry.getValue();
+          } catch (InterruptedException e) {
+            throw new IllegalStateException("No interruption in sequenced evaluation", e);
+          }
           // ctValue may be null if target was not successfully analyzed.
           if (ctValue != null) {
             ctValue.clear(!topLevelTargets.contains(ctValue.getConfiguredTarget()));
           }
         } else if (functionName.equals(SkyFunctions.ASPECT)) {
-          AspectValue aspectValue = (AspectValue) entry.getValue();
+          AspectValue aspectValue;
+          try {
+            aspectValue = (AspectValue) entry.getValue();
+          } catch (InterruptedException e) {
+            throw new IllegalStateException("No interruption in sequenced evaluation", e);
+          }
           // value may be null if target was not successfully analyzed.
           if (aspectValue != null) {
             aspectValue.clear(!topLevelAspects.contains(aspectValue));
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
index edb80f080c..82ddb17f76 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
@@ -44,4 +44,6 @@ interface InMemoryGraph extends ProcessableGraph {
 
   // Only for use by MemoizingEvaluator#delete
   Map<SkyKey, ? extends NodeEntry> getAllValues();
+
+  Map<SkyKey, ? extends NodeEntry> getAllValuesMutable();
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
index 71c3ad34c1..d85ab0013f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
@@ -120,6 +120,11 @@ public class InMemoryGraphImpl implements InMemoryGraph {
     return Collections.unmodifiableMap(nodeMap);
   }
 
+  @Override
+  public Map<SkyKey, ? extends NodeEntry> getAllValuesMutable() {
+    return nodeMap;
+  }
+
   @VisibleForTesting
   protected ConcurrentMap<SkyKey, ? extends NodeEntry> getNodeMap() {
     return nodeMap;
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index 68998b3a31..4ebde56624 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -258,6 +258,11 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
   }
 
   @Override
+  public Map<SkyKey, ? extends NodeEntry> getGraphMap() {
+    return graph.getAllValuesMutable();
+  }
+
+  @Override
   public Map<SkyKey, SkyValue> getDoneValues() {
     return graph.getDoneValues();
   }
diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index d690c4ada4..6bd01b538f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -91,6 +91,13 @@ public interface MemoizingEvaluator {
   Map<SkyKey, SkyValue> getValues();
 
   /**
+   * Returns the node entries in the graph. Should only be called between evaluations. The returned
+   * map is mutable, but do not mutate it unless you know what you are doing! Naively deleting an
+   * entry will break graph invariants and cause a crash.
+   */
+  Map<SkyKey, ? extends NodeEntry> getGraphMap();
+
+  /**
    * Returns the done (without error) values in the graph.
    *
    * <p>The returned map may be a live view of the graph.
diff --git a/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java
index 79904d0bff..786a527013 100644
--- a/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java
+++ b/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java
@@ -72,4 +72,9 @@ class DeterministicInMemoryGraph extends DeterministicHelper.DeterministicProces
   public Map<SkyKey, ? extends NodeEntry> getAllValues() {
     return ((InMemoryGraph) delegate).getAllValues();
   }
+
+  @Override
+  public Map<SkyKey, ? extends NodeEntry> getAllValuesMutable() {
+    return ((InMemoryGraph) delegate).getAllValuesMutable();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
index 31242dac47..f1feac2df3 100644
--- a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
+++ b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
@@ -67,4 +67,9 @@ class NotifyingInMemoryGraph extends NotifyingHelper.NotifyingProcessableGraph
   public Map<SkyKey, ? extends NodeEntry> getAllValues() {
     return ((InMemoryGraph) delegate).getAllValues();
   }
+
+  @Override
+  public Map<SkyKey, ? extends NodeEntry> getAllValuesMutable() {
+    return ((InMemoryGraph) delegate).getAllValuesMutable();
+  }
 }
diff --git a/src/test/shell/integration/discard_graph_edges_test.sh b/src/test/shell/integration/discard_graph_edges_test.sh
index ba104f3387..720f25d252 100755
--- a/src/test/shell/integration/discard_graph_edges_test.sh
+++ b/src/test/shell/integration/discard_graph_edges_test.sh
@@ -115,6 +115,85 @@ EOF
   [[ -e "bazel-bin/foo/dep.out.aspect" ]] || fail "Aspect bar not run"
 }
 
+function extract_histogram_count() {
+  local histofile="$1"
+  local item="$2"
+  # We can't use + here because Macs don't recognize it as a special character by default.
+  grep "$item" "$histofile" | sed -e 's/^ *[0-9][0-9]*: *\([0-9][0-9]*\) .*$/\1/' \
+      || fail "Couldn't get item from $histofile"
+}
+
+function prepare_histogram() {
+  readonly local build_args="$1"
+  rm -rf histodump
+  mkdir -p histodump || fail "Couldn't create directory"
+  readonly local server_pid_fifo="$TEST_TMPDIR/server_pid"
+cat > histodump/foo.bzl <<'EOF' || fail "Couldn't create bzl file"
+def foo():
+  pass
+EOF
+cat > histodump/bar.bzl <<'EOF' || fail "Couldn't create bzl file"
+def bar():
+  pass
+EOF
+cat > histodump/baz.bzl <<'EOF' || fail "Couldn't create bzl file"
+def baz():
+  pass
+EOF
+
+  cat > histodump/BUILD <<EOF || fail "Couldn't create BUILD file"
+load(":foo.bzl", "foo")
+load(":bar.bzl", "bar")
+load(":baz.bzl", "baz")
+genrule(name = 'histodump',
+        srcs = glob(["*.in"]),
+        outs = ['histo.txt'],
+        local = 1,
+        cmd = 'set -x; ${bazel_javabase}/bin/jmap -histo:live \$\$(cat $server_pid_fifo) > \$(location histo.txt)'
+       )
+EOF
+  rm -f "$server_pid_fifo"
+  mkfifo "$server_pid_fifo"
+  histo_file="$(bazel info "${PRODUCT_NAME}-genfiles" \
+      2> /dev/null)/histodump/histo.txt"
+  bazel clean >& "$TEST_log" || fail "Couldn't clean"
+  bazel $STARTUP_FLAGS build $build_args //histodump:histodump >& "$TEST_log" &
+  server_pid=$!
+  echo "$server_pid" > "$server_pid_fifo"
+  # Wait for previous command to finish.
+  wait "$server_pid" || fail "Bazel command failed"
+  cat "$histo_file" >> "$TEST_log"
+  echo "$histo_file"
+}
+
+function test_packages_cleared() {
+  local histo_file="$(prepare_histogram "--nodiscard_analysis_cache")"
+  local package_count="$(extract_histogram_count "$histo_file" \
+      'devtools\.build\.lib\..*\.Package$')"
+  [[ "$package_count" -ge 10 ]] \
+      || fail "package count $package_count too low: did you move/rename the class?"
+  local glob_count="$(extract_histogram_count "$histo_file" "GlobValue")"
+  [[ "$glob_count" -ge 30 ]] \
+      || fail "glob count $glob_count too low: did you move/rename the class?"
+  local env_count="$(extract_histogram_count "$histo_file" \
+      'Environment\$Extension$')"
+  [[ "$env_count" -ge 3 ]] \
+      || fail "env extension count $env_count too low: did you move/rename the class?"
+  local histo_file="$(prepare_histogram "$BUILD_FLAGS")"
+  package_count="$(extract_histogram_count "$histo_file" \
+      'devtools\.build\.lib\..*\.Package$')"
+  # A few packages aren't cleared.
+  [[ "$package_count" -le 8 ]] \
+      || fail "package count $package_count too high"
+  glob_count="$(extract_histogram_count "$histo_file" "GlobValue")"
+  [[ "$glob_count" -le 1 ]] \
+      || fail "glob count $glob_count too high"
+  env_count="$(extract_histogram_count "$histo_file" \
+      'Environment\$Extension$')"
+  [[ "$env_count" -le 2 ]] \
+      || fail "env extension count $env_count too high"
+}
+
 # Action conflicts can cause deletion of nodes, and deletion is tricky with no edges.
 function test_action_conflict() {
   mkdir -p conflict || fail "Couldn't create directory"