Skip to content

Commit

Permalink
Update shouldDeleteOnAnalysisInvalidatingChange to avoid invalidati…
Browse files Browse the repository at this point in the history
…ng the special empty configuration and related actions.

Work towards platform-based flags: bazelbuild#19409.

PiperOrigin-RevId: 623208595
Change-Id: I2af23afe893599e5ac7a338e5e09680495d1faa2
  • Loading branch information
katre authored and Kila2 committed May 13, 2024
1 parent a3fcc39 commit f4f26b7
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 15 deletions.
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -67,6 +69,13 @@ public NestedSet<Package> 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -77,6 +78,13 @@ public NestedSet<Package> 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

/**
Expand Down
8 changes: 4 additions & 4 deletions src/test/shell/integration/discard_analysis_cache_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
6 changes: 3 additions & 3 deletions src/test/shell/integration/discard_graph_edges_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 ]] \
Expand All @@ -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 ]] \
Expand Down

0 comments on commit f4f26b7

Please sign in to comment.