From 31654bd43743440398cca607757ca99e2a64da20 Mon Sep 17 00:00:00 2001 From: janakr Date: Wed, 29 Mar 2017 23:05:50 +0000 Subject: Drop loading-phase values if --discard_analysis_cache is true and we're not keeping incremental state. PiperOrigin-RevId: 151639711 --- .../lib/skyframe/SequencedSkyframeExecutor.java | 53 ++++++++++++--- .../devtools/build/skyframe/InMemoryGraph.java | 2 + .../devtools/build/skyframe/InMemoryGraphImpl.java | 5 ++ .../build/skyframe/InMemoryMemoizingEvaluator.java | 5 ++ .../build/skyframe/MemoizingEvaluator.java | 7 ++ .../build/skyframe/DeterministicInMemoryGraph.java | 5 ++ .../build/skyframe/NotifyingInMemoryGraph.java | 5 ++ .../shell/integration/discard_graph_edges_test.sh | 79 ++++++++++++++++++++++ 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 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. * - *

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. + *

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. + * + *

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). * - *

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). + *

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 topLevelTargets, Collection 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 entry : memoizingEvaluator.getValues().entrySet()) { - SkyFunctionName functionName = entry.getKey().functionName(); + Iterator> it = + memoizingEvaluator.getGraphMap().entrySet().iterator(); + while (it.hasNext()) { + Map.Entry 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 getAllValues(); + + Map 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 getAllValuesMutable() { + return nodeMap; + } + @VisibleForTesting protected ConcurrentMap 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 @@ -257,6 +257,11 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator { return graph.getValues(); } + @Override + public Map getGraphMap() { + return graph.getAllValuesMutable(); + } + @Override public Map 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 @@ -90,6 +90,13 @@ public interface MemoizingEvaluator { // require some care because getValues gives access to the previous value for changed/dirty nodes. Map 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 getGraphMap(); + /** * Returns the done (without error) values in 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 getAllValues() { return ((InMemoryGraph) delegate).getAllValues(); } + + @Override + public Map 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 getAllValues() { return ((InMemoryGraph) delegate).getAllValues(); } + + @Override + public Map 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 < /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" -- cgit v1.2.3