Avoid re-clearing the analysis cache (and saying so) after cleans.
1) After cleaning, don't bother checking the configurations - because the old ones were lost in the Skyframe graph being cleared, the ones we have now are definitely different (if possibly equivalent) instances, but we don't care because the graph is empty already, so there's nothing to clean up.
2) After cleaning, the fact that the last completed build was discard_analysis_cache is no longer relevant. The graph was completely cleared already, so there's again nothing to clean up.
3) When the last completed build was discard_analysis_cache, it doesn't matter what configuration we have, so there's no point in doing a potentially lengthy diff whose result we don't care about.
4) When the last completed build was discard_analysis_cache, we do throw away the analysis cache at the start of the next one. But not because the build options have changed. Print a different message in that case.
This is largely a cosmetic change - the graph was already empty, so clearing it should have been pretty trivial. But the message being printed is misleading.
RELNOTES: None.
PiperOrigin-RevId: 210760599
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index a86d51a..cc1c773 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -211,10 +211,15 @@
@VisibleForTesting
public void setConfigurations(
EventHandler eventHandler, BuildConfigurationCollection configurations) {
- // Clear all cached ConfiguredTargets on configuration change of if --discard_analysis_cache
- // was set on the previous build. In the former case, it's not required for correctness, but
- // prevents unbounded memory usage.
- if (this.areConfigurationsDifferent(configurations) || skyframeAnalysisWasDiscarded) {
+ if (skyframeAnalysisWasDiscarded) {
+ eventHandler.handle(
+ Event.info(
+ "--discard_analysis_cache was used in the previous build, "
+ + "discarding analysis cache."));
+ skyframeExecutor.handleConfiguredTargetChange();
+ } else if (this.areConfigurationsDifferent(configurations)) {
+ // Clearing cached ConfiguredTargets when the configuration changes is not required for
+ // correctness, but prevents unbounded memory usage.
eventHandler.handle(Event.info("Build options have changed, discarding analysis cache."));
skyframeExecutor.handleConfiguredTargetChange();
}
@@ -703,6 +708,16 @@
}
/**
+ * Clears any data cached in this BuildView. To be called when the attached SkyframeExecutor is
+ * reset.
+ */
+ void reset() {
+ configurations = null;
+ skyframeAnalysisWasDiscarded = false;
+ clearLegacyData();
+ }
+
+ /**
* Hack to invalidate actions in legacy action graph when their values are invalidated in
* skyframe.
*/
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 4f2f8bb..33a9cc8 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
@@ -728,7 +728,7 @@
public void resetEvaluator() {
init();
emittedEventState.clear();
- skyframeBuildView.clearLegacyData();
+ skyframeBuildView.reset();
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
index ebf06fb..cbb3b33 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -912,4 +912,150 @@
.put("//test:shared", 0)
.build());
}
+
+ @Test
+ public void noCacheClearMessageAfterCleanWithSameOptions() throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration();
+ update("//test:top");
+ cleanSkyframe();
+ eventCollector.clear();
+ update("//test:top");
+ assertNoEvents();
+ }
+
+ @Test
+ public void noCacheClearMessageAfterCleanWithDifferentOptions() throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--definitely_relevant=before");
+ update("//test:top");
+ cleanSkyframe();
+ useConfiguration("--definitely_relevant=after");
+ eventCollector.clear();
+ update("//test:top");
+ assertNoEvents();
+ }
+
+ @Test
+ public void noCacheClearMessageAfterDiscardAnalysisCacheThenCleanWithSameOptions()
+ throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--discard_analysis_cache");
+ update("//test:top");
+ cleanSkyframe();
+ eventCollector.clear();
+ update("//test:top");
+ assertNoEvents();
+ }
+
+ @Test
+ public void noCacheClearMessageAfterDiscardAnalysisCacheThenCleanWithChangedOptions()
+ throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--definitely_relevant=before", "--discard_analysis_cache");
+ update("//test:top");
+ cleanSkyframe();
+ useConfiguration("--definitely_relevant=after", "--discard_analysis_cache");
+ eventCollector.clear();
+ update("//test:top");
+ assertNoEvents();
+ }
+
+ @Test
+ public void cacheClearMessageAfterDiscardAnalysisCacheBuild() throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--probably_irrelevant=yeah", "--discard_analysis_cache");
+ update("//test:top");
+ eventCollector.clear();
+ update("//test:top");
+ assertContainsEvent("--discard_analysis_cache");
+ assertDoesNotContainEvent("Build options have changed");
+ assertContainsEvent("discarding analysis cache");
+ }
+
+ @Test
+ public void noCacheClearMessageAfterNonDiscardAnalysisCacheBuild() throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--discard_analysis_cache");
+ update("//test:top");
+ useConfiguration();
+ update("//test:top");
+ eventCollector.clear();
+ update("//test:top");
+ assertNoEvents();
+ }
+
+ @Test
+ public void noCacheClearMessageAfterIrrelevantOptionChanges() throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--probably_irrelevant=old");
+ update("//test:top");
+ useConfiguration("--probably_irrelevant=new");
+ eventCollector.clear();
+ update("//test:top");
+ assertNoEvents();
+ }
+
+ @Test
+ public void cacheClearMessageAfterRelevantOptionChanges() throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--definitely_relevant=old");
+ update("//test:top");
+ useConfiguration("--definitely_relevant=new");
+ eventCollector.clear();
+ update("//test:top");
+ assertDoesNotContainEvent("--discard_analysis_cache");
+ assertContainsEvent("Build options have changed");
+ assertContainsEvent("discarding analysis cache");
+ }
+
+ @Test
+ public void cacheClearMessageAfterDiscardAnalysisCacheBuildWithRelevantOptionChanges()
+ throws Exception {
+ setupDiffResetTesting();
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'normal_lib')",
+ "normal_lib(name='top')");
+ useConfiguration("--discard_analysis_cache", "--definitely_relevant=old");
+ update("//test:top");
+ useConfiguration("--discard_analysis_cache", "--definitely_relevant=new");
+ eventCollector.clear();
+ update("//test:top");
+ assertContainsEvent("--discard_analysis_cache");
+ assertDoesNotContainEvent("Build options have changed");
+ assertContainsEvent("discarding analysis cache");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index eb4b9cf..0dd2bec 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -201,7 +201,12 @@
useConfiguration();
skyframeExecutor =
createSkyframeExecutor(pkgFactory, ruleClassProvider.getBuildInfoFactories());
+ reinitializeSkyframeExecutor();
+ packageManager = skyframeExecutor.getPackageManager();
+ buildView = new BuildViewForTesting(directories, ruleClassProvider, skyframeExecutor, null);
+ }
+ private void reinitializeSkyframeExecutor() {
TestConstants.processSkyframeExecutorForTesting(skyframeExecutor);
PackageCacheOptions packageCacheOptions = Options.getDefaults(PackageCacheOptions.class);
packageCacheOptions.showLoadingProgress = true;
@@ -210,7 +215,7 @@
pkgLocator,
packageCacheOptions,
Options.getDefaults(SkylarkSemanticsOptions.class),
- ruleClassProvider.getDefaultsPackageContent(
+ this.ruleClassProvider.getDefaultsPackageContent(
analysisMock.getInvocationPolicyEnforcer().getInvocationPolicy()),
UUID.randomUUID(),
ImmutableMap.<String, String>of(),
@@ -224,10 +229,12 @@
PrecomputedValue.injected(
RepositoryDelegatorFunction.DEPENDENCY_FOR_UNCONDITIONAL_FETCHING,
RepositoryDelegatorFunction.DONT_FETCH_UNCONDITIONALLY)));
+ }
- packageManager = skyframeExecutor.getPackageManager();
- buildView = new BuildViewForTesting(directories, ruleClassProvider, skyframeExecutor, null);
-
+ /** Resets the SkyframeExecutor, as if a clean had been executed. */
+ protected void cleanSkyframe() {
+ skyframeExecutor.resetEvaluator();
+ reinitializeSkyframeExecutor();
}
protected AnalysisMock getAnalysisMock() {
@@ -319,6 +326,7 @@
AnalysisOptions viewOptions = optionsParser.getOptions(AnalysisOptions.class);
// update --keep_going option if test requested it.
boolean keepGoing = flags.contains(Flag.KEEP_GOING);
+ boolean discardAnalysisCache = viewOptions.discardAnalysisCache;
viewOptions.skyframePrepareAnalysis = flags.contains(Flag.SKYFRAME_PREPARE_ANALYSIS);
PackageCacheOptions packageCacheOptions = optionsParser.getOptions(PackageCacheOptions.class);
@@ -375,6 +383,9 @@
AnalysisTestUtil.TOP_LEVEL_ARTIFACT_CONTEXT,
reporter,
eventBus);
+ if (discardAnalysisCache) {
+ buildView.clearAnalysisCache(analysisResult.getTargetsToBuild(), analysisResult.getAspects());
+ }
masterConfig = analysisResult.getConfigurationCollection();
return analysisResult;
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index fbb2944..7d059a6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -77,6 +77,7 @@
import com.google.devtools.build.lib.packages.RawAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.skyframe.AspectValue;
import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
@@ -575,4 +576,10 @@
}
return null;
}
+
+ /** Clears the analysis cache as in --discard_analysis_cache. */
+ public void clearAnalysisCache(
+ Collection<ConfiguredTarget> topLevelTargets, Collection<AspectValue> topLevelAspects) {
+ skyframeBuildView.clearAnalysisCache(topLevelTargets, topLevelAspects);
+ }
}
