Let ConfiguredRuleClassProvider decide whether to drop the analysis cache.

ConfiguredRuleClassProvider can specify a predicate which accepts an
OptionsDiff and the new BuildOptions in order to make a decision on whether
the given diff requires the analysis cache to be dropped.

This predicate is only called if all of the following hold:
* there was an old configuration collection (if not, the cache is not
  dropped because there wasn't one yet)
* the old configuration collection is not exactly equal to the new
  configuration collection (if it is, the cache is not dropped because
  it definitely hasn't changed)
* the old configuration collection has the same number of configurations
  as the new collection (if not, the cache is always dropped because
  experimental_multi_cpu has changed, definitely not a flag which should
  cause old analysis cache to stick around!)

If all of these hold, the old target configurations are paired up with
the new target configurations by index in the configuration collection,
and each pair is diffed. The predicate is called with each diff and the
corresponding new configuration's build options. If any of these
invocations returns true, the cache is dropped. Otherwise, the cache is
kept for the next build.

No implementation of this predicate is actually supplied for this
change, so the old behavior (always drop the cache if the configuration
changes at all) holds.

RELNOTES: None.
PiperOrigin-RevId: 201050049

2 files changed, 86 insertions, 2 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index 0831578ee4..0ea36aa52a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
 import com.google.devtools.build.lib.analysis.config.ComposingRuleTransitionFactory;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.DefaultsPackage;
@@ -88,6 +89,17 @@ import javax.annotation.Nullable;
 public class ConfiguredRuleClassProvider implements RuleClassProvider {
 
   /**
+   * Predicate for determining whether the analysis cache should be cleared, given the new set of
+   * build options and the diff from the old set.
+   */
+  @FunctionalInterface
+  public interface OptionsDiffPredicate {
+    public static final OptionsDiffPredicate ALWAYS_INVALIDATE = (diff, options) -> true;
+
+    public boolean apply(OptionsDiff diff, BuildOptions newOptions);
+  }
+
+  /**
    * Custom dependency validation logic.
    */
   public interface PrerequisiteValidator {
@@ -236,6 +248,8 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
     private List<Class<? extends BuildConfiguration.Fragment>> universalFragments =
         new ArrayList<>();
     @Nullable private RuleTransitionFactory trimmingTransitionFactory;
+    private OptionsDiffPredicate shouldInvalidateCacheForDiff =
+        OptionsDiffPredicate.ALWAYS_INVALIDATE;
     private PrerequisiteValidator prerequisiteValidator;
     private ImmutableList.Builder<Bootstrap> skylarkBootstraps =
         ImmutableList.<Bootstrap>builder();
@@ -419,6 +433,30 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
       return this.addTrimmingTransitionFactory(factory);
     }
 
+    /**
+     * Sets the predicate which determines whether the analysis cache should be invalidated for the
+     * given options diff.
+     */
+    public Builder setShouldInvalidateCacheForDiff(
+        OptionsDiffPredicate shouldInvalidateCacheForDiff) {
+      Preconditions.checkState(
+          this.shouldInvalidateCacheForDiff.equals(OptionsDiffPredicate.ALWAYS_INVALIDATE),
+          "Cache invalidation function was already set");
+      this.shouldInvalidateCacheForDiff = shouldInvalidateCacheForDiff;
+      return this;
+    }
+
+    /**
+     * Overrides the predicate which determines whether the analysis cache should be invalidated for
+     * the given options diff.
+     */
+    @VisibleForTesting(/* for testing cache invalidation without relying on prod use */ )
+    public Builder overrideShouldInvalidateCacheForDiffForTesting(
+        OptionsDiffPredicate shouldInvalidateCacheForDiff) {
+      this.shouldInvalidateCacheForDiff = OptionsDiffPredicate.ALWAYS_INVALIDATE;
+      return this.setShouldInvalidateCacheForDiff(shouldInvalidateCacheForDiff);
+    }
+
     private RuleConfiguredTargetFactory createFactory(
         Class<? extends RuleConfiguredTargetFactory> factoryClass) {
       try {
@@ -494,6 +532,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
           ImmutableList.copyOf(configurationFragmentFactories),
           ImmutableList.copyOf(universalFragments),
           trimmingTransitionFactory,
+          shouldInvalidateCacheForDiff,
           prerequisiteValidator,
           skylarkAccessibleTopLevels.build(),
           skylarkBootstraps.build(),
@@ -594,6 +633,9 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
   /** The transition factory used to produce the transition that will trim targets. */
   @Nullable private final RuleTransitionFactory trimmingTransitionFactory;
 
+  /** The predicate used to determine whether a diff requires the cache to be invalidated. */
+  private final OptionsDiffPredicate shouldInvalidateCacheForDiff;
+
   /**
    * Configuration fragments that should be available to all rules even when they don't
    * explicitly require it.
@@ -628,6 +670,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
       ImmutableList<ConfigurationFragmentFactory> configurationFragments,
       ImmutableList<Class<? extends BuildConfiguration.Fragment>> universalFragments,
       @Nullable RuleTransitionFactory trimmingTransitionFactory,
+      OptionsDiffPredicate shouldInvalidateCacheForDiff,
       PrerequisiteValidator prerequisiteValidator,
       ImmutableMap<String, Object> skylarkAccessibleJavaClasses,
       ImmutableList<Bootstrap> skylarkBootstraps,
@@ -647,6 +690,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
     this.configurationFragmentFactories = configurationFragments;
     this.universalFragments = universalFragments;
     this.trimmingTransitionFactory = trimmingTransitionFactory;
+    this.shouldInvalidateCacheForDiff = shouldInvalidateCacheForDiff;
     this.prerequisiteValidator = prerequisiteValidator;
     this.globals = createGlobals(skylarkAccessibleJavaClasses, skylarkBootstraps);
     this.reservedActionMnemonics = reservedActionMnemonics;
@@ -715,6 +759,11 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
     return trimmingTransitionFactory;
   }
 
+  /** Returns whether the analysis cache should be invalidated for the given options diff. */
+  public boolean shouldInvalidateCacheForDiff(OptionsDiff diff, BuildOptions newOptions) {
+    return shouldInvalidateCacheForDiff.apply(diff, newOptions);
+  }
+
   /**
    * Returns the set of configuration options that are supported in this module.
    */
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 bf14785ca5..acb7c4a064 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
@@ -43,6 +43,8 @@ import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory.BuildInfoKey;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
@@ -146,6 +148,40 @@ public final class SkyframeBuildView {
     return factory;
   }
 
+  private boolean areConfigurationsDifferent(BuildConfigurationCollection configurations) {
+    if (this.configurations == null) {
+      // no configurations currently, no need to drop anything
+      return false;
+    }
+    if (configurations.equals(this.configurations)) {
+      // exact same configurations, no need to drop anything
+      return false;
+    }
+    if (configurations.getTargetConfigurations().size()
+        != this.configurations.getTargetConfigurations().size()) {
+      // some option that changes the number of configurations has changed, that's definitely not
+      // any of the options that are okay to change
+      return true;
+    }
+    // Here we assume that the configurations will appear in the same order between invocations -
+    // which is the case today, because the only way to have multiple configurations is to use
+    // --experimental_multi_cpu, which creates configurations in sorted order of cpu value.
+    // Those configurations are all identical except for their cpu values, so the configuration we
+    // compare against only matters for making sure the cpu matches. Which we do care about.
+    for (int configIndex = 0;
+        configIndex < configurations.getTargetConfigurations().size();
+        configIndex += 1) {
+      BuildConfiguration oldConfig = this.configurations.getTargetConfigurations().get(configIndex);
+      BuildConfiguration newConfig = configurations.getTargetConfigurations().get(configIndex);
+      OptionsDiff diff = BuildOptions.diff(oldConfig.getOptions(), newConfig.getOptions());
+      if (ruleClassProvider.shouldInvalidateCacheForDiff(diff, newConfig.getOptions())) {
+        return true;
+      }
+    }
+    // We don't need to check the host configuration because it's derived from the target options.
+    return false;
+  }
+
   /** Sets the configurations. Not thread-safe. DO NOT CALL except from tests! */
   @VisibleForTesting
   public void setConfigurations(
@@ -153,8 +189,7 @@ public final class SkyframeBuildView {
     // 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.configurations != null && !configurations.equals(this.configurations))
-        || skyframeAnalysisWasDiscarded) {
+    if (this.areConfigurationsDifferent(configurations) || skyframeAnalysisWasDiscarded) {
       eventHandler.handle(Event.info("Build options have changed, discarding analysis cache."));
       skyframeExecutor.handleConfiguredTargetChange();
     }