From f4f26b747ad967d6c6b3521e70b12cda2da72883 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 9 Apr 2024 10:23:37 -0700 Subject: [PATCH] Update `shouldDeleteOnAnalysisInvalidatingChange` to avoid invalidating the special empty configuration and related actions. Work towards platform-based flags: #19409. PiperOrigin-RevId: 623208595 Change-Id: I2af23afe893599e5ac7a338e5e09680495d1faa2 --- .../google/devtools/build/lib/skyframe/BUILD | 2 + .../NonRuleConfiguredTargetValue.java | 9 +++++ .../skyframe/RuleConfiguredTargetValue.java | 8 ++++ .../skyframe/SequencedSkyframeExecutor.java | 38 +++++++++++++++---- .../discard_analysis_cache_test.sh | 8 ++-- .../integration/discard_graph_edges_test.sh | 6 +-- 6 files changed, 56 insertions(+), 15 deletions(-) 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 f61f16b603adf3..325ec138320e06 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -237,6 +237,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/additional_configuration_change_event", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", + "//src/main/java/com/google/devtools/build/lib/analysis:config/common_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", @@ -3129,6 +3130,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", + "//src/main/java/com/google/devtools/build/lib/analysis:config/common_options", "//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:rule_configured_object_value", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java index 446ca39a272f1c..cfaaaee213f168 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.devtools.build.lib.analysis.config.CommonOptions.EMPTY_OPTIONS; + import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -67,6 +69,13 @@ public NestedSet getTransitivePackages() { @Override public void clear(boolean clearEverything) { + if (configuredTarget != null + && configuredTarget.getConfigurationKey() != null + && configuredTarget.getConfigurationChecksum().equals(EMPTY_OPTIONS.checksum())) { + // Keep these to avoid the need to re-create them later, they are dependencies of the empty + // configuration key and will never change. + return; + } if (clearEverything) { configuredTarget = null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java index 860ba3fb564212..3a8ae0dca09849 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.devtools.build.lib.analysis.config.CommonOptions.EMPTY_OPTIONS; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; @@ -77,6 +78,13 @@ public NestedSet getTransitivePackages() { @Override public void clear(boolean clearEverything) { + if (configuredTarget != null + && configuredTarget.getConfigurationKey() != null + && configuredTarget.getConfigurationChecksum().equals(EMPTY_OPTIONS.checksum())) { + // Keep these to avoid the need to re-create them later, they are dependencies of the empty + // configuration key and will never change. + return; + } if (clearEverything) { configuredTarget = null; } 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 d7b2c20f9638ce..3f45d40ae05422 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 @@ -15,6 +15,7 @@ import static com.google.common.base.Throwables.throwIfInstanceOf; import static com.google.common.base.Throwables.throwIfUnchecked; +import static com.google.devtools.build.lib.analysis.config.CommonOptions.EMPTY_OPTIONS; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -646,16 +647,37 @@ public void handleAnalysisInvalidatingChange() { memoizingEvaluator.delete(this::shouldDeleteOnAnalysisInvalidatingChange); } - // Also remove ActionLookupData since all such nodes depend on ActionLookupKey nodes and deleting - // en masse is cheaper than deleting via graph traversal (b/192863968). @ForOverride protected boolean shouldDeleteOnAnalysisInvalidatingChange(SkyKey k) { - return k instanceof ArtifactNestedSetKey - || k instanceof ActionLookupKey - || (k instanceof BuildConfigurationKey - && getSkyframeBuildView().getBuildConfiguration() != null - && !k.equals(getSkyframeBuildView().getBuildConfiguration().getKey())) - || k instanceof ActionLookupData; + // TODO: b/330770905 - Rewrite this to use pattern matching when available. + // Also remove ActionLookupData since all such nodes depend on ActionLookupKey nodes and + // deleting en masse is cheaper than deleting via graph traversal (b/192863968). + if (k instanceof ArtifactNestedSetKey || k instanceof ActionLookupData) { + return true; + } + // Remove BuildConfigurationKeys except for the currently active key and the key for + // EMPTY_OPTIONS, which is a constant and will be re-used frequently. + if (k instanceof BuildConfigurationKey key) { + if (key.getOptionsChecksum().equals(EMPTY_OPTIONS.checksum())) { + return false; + } + if (getSkyframeBuildView().getBuildConfiguration() != null + && k.equals(getSkyframeBuildView().getBuildConfiguration().getKey())) { + return false; + } + return true; + } + // Remove ActionLookupKeys unless they are for the empty options config, in which case they will + // be re-used frequently and we can avoid re-creating them. They are dependencies of the empty + // configuration key and will never change. + if (k instanceof ActionLookupKey lookupKey) { + BuildConfigurationKey key = lookupKey.getConfigurationKey(); + if (key != null && key.getOptionsChecksum().equals(EMPTY_OPTIONS.checksum())) { + return false; + } + return true; + } + return false; } /** diff --git a/src/test/shell/integration/discard_analysis_cache_test.sh b/src/test/shell/integration/discard_analysis_cache_test.sh index 7ea9438c0f2363..52db30d118037f 100755 --- a/src/test/shell/integration/discard_analysis_cache_test.sh +++ b/src/test/shell/integration/discard_analysis_cache_test.sh @@ -168,11 +168,11 @@ EOF bazel build --discard_analysis_cache //foo:foo >& "$TEST_log" \ || fail "Expected success" "$jmaptool" -histo:live "$server_pid" > histo.txt - cat histo.txt >> "$TEST_log" + #cat histo.txt >> "$TEST_log" ct_count="$(extract_histogram_count histo.txt 'RuleConfiguredTarget$')" aspect_count="$(extract_histogram_count histo.txt 'lib.packages.Aspect$')" - # One top-level configured target is allowed to stick around. - [[ "$ct_count" -le 1 ]] \ + # Several top-level configured targets are allowed to stick around. + [[ "$ct_count" -le 17 ]] \ || fail "Too many configured targets: $ct_count" [[ "$aspect_count" -eq 0 ]] || fail "Too many aspects: $aspect_count" bazel --batch clean >& "$TEST_log" || fail "Expected success" @@ -191,7 +191,7 @@ EOF aspect_count="$(extract_histogram_count histo.txt 'lib.packages.Aspect$')" # One top-level aspect is allowed to stick around. [[ "$aspect_count" -le 1 ]] || fail "Too many aspects: $aspect_count" - [[ "$ct_count" -le 1 ]] || fail "Too many configured targets: $ct_count" + [[ "$ct_count" -le 17 ]] || fail "Too many configured targets: $ct_count" } run_suite "test for --discard_analysis_cache" diff --git a/src/test/shell/integration/discard_graph_edges_test.sh b/src/test/shell/integration/discard_graph_edges_test.sh index c286c60bb95223..15cc685a7753f8 100755 --- a/src/test/shell/integration/discard_graph_edges_test.sh +++ b/src/test/shell/integration/discard_graph_edges_test.sh @@ -271,7 +271,7 @@ function test_packages_cleared() { package_count="$(extract_histogram_count "$histo_file" \ 'devtools\.build\.lib\..*\.Package$')" # A few packages aren't cleared. - [[ "$package_count" -le 22 ]] \ + [[ "$package_count" -le 25 ]] \ || fail "package count $package_count too high" glob_count="$(extract_histogram_count "$histo_file" "GlobValueWithImmutableSet$")" [[ "$glob_count" -le 1 ]] \ @@ -281,8 +281,8 @@ function test_packages_cleared() { || fail "Module count $module_count too high" ct_count="$(extract_histogram_count "$histo_file" \ 'RuleConfiguredTarget$')" - [[ "$ct_count" -le 1 ]] \ - || fail "too many RuleConfiguredTarget: expected at most 1, got $ct_count" + [[ "$ct_count" -le 40 ]] \ + || fail "too many RuleConfiguredTarget: expected at most 40, got $ct_count" non_incremental_entry_count="$(extract_histogram_count "$histo_file" \ '\.NonIncrementalInMemoryNodeEntry$')" [[ "$non_incremental_entry_count" -ge 100 ]] \