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

1 files changed, 371 insertions, 1 deletions

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 9d14119cd7..ebf06fbfd8 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
@@ -14,21 +14,42 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.ImmutableMultiset.toImmutableMultiset;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultiset;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.MoreCollectors;
 import com.google.devtools.build.lib.actions.Action;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
+import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.util.AnalysisCachingTestBase;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.rules.java.JavaInfo;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.skyframe.AspectValue;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.testutil.Suite;
 import com.google.devtools.build.lib.testutil.TestConstants.InternalTestExecutionMode;
+import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.testutil.TestSpec;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDefinition;
+import com.google.devtools.common.options.OptionDocumentationCategory;
+import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.OptionsParser;
+import java.util.Map;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -226,7 +247,7 @@ public class AnalysisCachingTest extends AnalysisCachingTestBase {
     update(defaultFlags().with(Flag.KEEP_GOING), "//conflict:x", "//conflict:_objs/x/foo.pic.o");
     assertNoEvents();
   }
-  
+
   /**
    * For two conflicting actions whose primary inputs are different, no list diff detail should be
    * part of the output.
@@ -542,4 +563,353 @@ public class AnalysisCachingTest extends AnalysisCachingTestBase {
         .isEqualTo(1);
     assertThat(countObjectsPartiallyMatchingRegex(newAnalyzedTargets, "//java/a:y")).isEqualTo(1);
   }
+
+  /** Test options class for testing diff-based analysis cache resetting. */
+  public static final class DiffResetOptions extends FragmentOptions {
+    public static final PatchTransition CLEAR_IRRELEVANT =
+        (options) -> {
+          BuildOptions cloned = options.clone();
+          cloned.get(DiffResetOptions.class).probablyIrrelevantOption = "(cleared)";
+          return cloned;
+        };
+
+    @Option(
+        name = "probably_irrelevant",
+        defaultValue = "(unset)",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        effectTags = {OptionEffectTag.UNKNOWN},
+        help = "This option is irrelevant to non-uses_irrelevant targets and is trimmed from them.")
+    public String probablyIrrelevantOption;
+
+    @Option(
+        name = "definitely_relevant",
+        defaultValue = "(unset)",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        effectTags = {OptionEffectTag.UNKNOWN},
+        help = "This option is not trimmed and is used by all targets.")
+    public String definitelyRelevantOption;
+
+    @Option(
+        name = "host_relevant",
+        defaultValue = "(unset)",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        effectTags = {OptionEffectTag.UNKNOWN},
+        help = "This option is not trimmed and is used by all host targets.")
+    public String hostRelevantOption;
+
+    @Override
+    public DiffResetOptions getHost() {
+      DiffResetOptions host = ((DiffResetOptions) super.getHost());
+      host.definitelyRelevantOption = hostRelevantOption;
+      return host;
+    }
+  }
+
+  @SkylarkModule(name = "test_diff_fragment", doc = "fragment for testing differy fragments")
+  private static final class DiffResetFragment extends BuildConfiguration.Fragment {}
+
+  private static final class DiffResetFactory implements ConfigurationFragmentFactory {
+    @Override
+    public BuildConfiguration.Fragment create(BuildOptions options) {
+      return new DiffResetFragment();
+    }
+
+    @Override
+    public Class<? extends BuildConfiguration.Fragment> creates() {
+      return DiffResetFragment.class;
+    }
+
+    @Override
+    public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() {
+      return ImmutableSet.of(DiffResetOptions.class);
+    }
+  }
+
+  private void setupDiffResetTesting() throws Exception {
+    OptionDefinition probablyIrrelevantOption =
+        OptionsParser.getOptionDefinitions(DiffResetOptions.class)
+            .stream()
+            .filter(definition -> definition.getOptionName().equals("probably_irrelevant"))
+            .collect(MoreCollectors.onlyElement());
+    ImmutableSet<OptionDefinition> optionsThatCanChange = ImmutableSet.of(probablyIrrelevantOption);
+    ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
+    TestRuleClassProvider.addStandardRules(builder);
+    builder.addConfig(DiffResetOptions.class, new DiffResetFactory());
+    builder.overrideShouldInvalidateCacheForDiffForTesting(
+        (diff, newOptions) -> {
+          for (OptionDefinition changed : diff.getFirst().keySet()) {
+            if (!optionsThatCanChange.contains(changed)) {
+              return true;
+            }
+          }
+          return false;
+        });
+    builder.overrideTrimmingTransitionFactoryForTesting(
+        (rule) -> {
+          if (rule.getRuleClassObject().getName().equals("uses_irrelevant")) {
+            return NoTransition.INSTANCE;
+          }
+          return DiffResetOptions.CLEAR_IRRELEVANT;
+        });
+    useRuleClassProvider(builder.build());
+    scratch.file(
+        "test/lib.bzl",
+        "def _empty_impl(ctx):",
+        "  pass",
+        "normal_lib = rule(",
+        "    implementation = _empty_impl,",
+        "    fragments = ['test_diff_fragment'],",
+        "    attrs = {",
+        "        'deps': attr.label_list(),",
+        "        'host_deps': attr.label_list(cfg='host'),",
+        "    },",
+        ")",
+        "uses_irrelevant = rule(",
+        "    implementation = _empty_impl,",
+        "    fragments = ['test_diff_fragment'],",
+        "    attrs = {",
+        "        'deps': attr.label_list(),",
+        "        'host_deps': attr.label_list(cfg='host'),",
+        "    },",
+        ")");
+    update();
+  }
+
+  private void assertNumberOfAnalyzedConfigurationsOfTargets(
+      Map<String, Integer> targetsWithCounts) {
+    ImmutableMultiset<Label> actualSet =
+        getSkyframeEvaluatedTargetKeys()
+            .stream()
+            .filter(key -> key instanceof ConfiguredTargetKey)
+            .map(key -> ((ConfiguredTargetKey) key).getLabel())
+            .collect(toImmutableMultiset());
+    ImmutableMap<Label, Integer> expected =
+        targetsWithCounts
+            .entrySet()
+            .stream()
+            .collect(
+                toImmutableMap(
+                    entry -> Label.parseAbsoluteUnchecked(entry.getKey()),
+                    entry -> entry.getValue()));
+    ImmutableMap<Label, Integer> actual =
+        expected
+            .keySet()
+            .stream()
+            .collect(toImmutableMap(label -> label, label -> actualSet.count(label)));
+    assertThat(actual).containsExactlyEntriesIn(expected);
+  }
+
+  @Test
+  public void cacheNotClearedWhenOptionsStaySame() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--definitely_relevant=Testing");
+    update("//test:top");
+    update("//test:top");
+    // these targets were cached and did not need to be reanalyzed
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 0)
+            .put("//test:shared", 0)
+            .build());
+  }
+
+  @Test
+  public void cacheNotClearedWhenOptionsStaySameWithMultiCpu() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--experimental_multi_cpu=k8,ppc", "--definitely_relevant=Testing");
+    update("//test:top");
+    update("//test:top");
+    // these targets were cached and did not need to be reanalyzed
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 0)
+            .put("//test:shared", 0)
+            .build());
+  }
+
+  @Test
+  public void cacheClearedWhenNonAllowedOptionsChange() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--definitely_relevant=Test 1");
+    update("//test:top");
+    useConfiguration("--definitely_relevant=Test 2");
+    update("//test:top");
+    useConfiguration("--definitely_relevant=Test 1");
+    update("//test:top");
+    // these targets needed to be reanalyzed even though we built them in this configuration
+    // just a moment ago
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 1)
+            .put("//test:shared", 1)
+            .build());
+  }
+
+  @Test
+  public void cacheClearedWhenNonAllowedHostOptionsChange() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', host_deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--host_relevant=Test 1");
+    update("//test:top");
+    useConfiguration("--host_relevant=Test 2");
+    update("//test:top");
+    useConfiguration("--host_relevant=Test 1");
+    update("//test:top");
+    // these targets needed to be reanalyzed even though we built them in this configuration
+    // just a moment ago
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 1)
+            .put("//test:shared", 1)
+            .build());
+  }
+
+  @Test
+  public void cacheClearedWhenMultiCpuChanges() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--experimental_multi_cpu=k8,ppc");
+    update("//test:top");
+    useConfiguration("--experimental_multi_cpu=k8,armeabi-v7a");
+    update("//test:top");
+    // we needed to reanalyze these in both k8 and armeabi-v7a even though we did the k8 analysis
+    // just a moment ago as part of the previous build
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 2)
+            .put("//test:shared", 2)
+            .build());
+  }
+
+  @Test
+  public void cacheClearedWhenMultiCpuGetsBigger() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--experimental_multi_cpu=k8,ppc");
+    update("//test:top");
+    useConfiguration("--experimental_multi_cpu=k8,ppc,armeabi-v7a");
+    update("//test:top");
+    // we needed to reanalyze these in all of {k8,ppc,armeabi-v7a} even though we did the k8 and ppc
+    // analysis just a moment ago as part of the previous build
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 3)
+            .put("//test:shared", 3)
+            .build());
+  }
+
+  @Test
+  public void cacheClearedWhenMultiCpuGetsSmaller() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--experimental_multi_cpu=k8,ppc,armeabi-v7a");
+    update("//test:top");
+    useConfiguration("--experimental_multi_cpu=k8,ppc");
+    update("//test:top");
+    // we needed to reanalyze these in both k8 and ppc even though we did the k8 and ppc
+    // analysis just a moment ago as part of the previous build
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 2)
+            .put("//test:shared", 2)
+            .build());
+  }
+
+  @Test
+  public void cacheNotClearedWhenAllowedOptionsChange() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration("--definitely_relevant=Testing", "--probably_irrelevant=Test 1");
+    update("//test:top");
+    useConfiguration("--definitely_relevant=Testing", "--probably_irrelevant=Test 2");
+    update("//test:top");
+    // the shared library got to reuse the cached value, while the entry point had to be rebuilt in
+    // the new configuration
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 1)
+            .put("//test:shared", 0)
+            .build());
+    useConfiguration("--definitely_relevant=Testing", "--probably_irrelevant=Test 1");
+    update("//test:top");
+    // now we're back to the old configuration with no cache clears, so no work needed to be done
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 0)
+            .put("//test:shared", 0)
+            .build());
+  }
+
+  @Test
+  public void cacheNotClearedWhenAllowedOptionsChangeWithMultiCpu() throws Exception {
+    setupDiffResetTesting();
+    scratch.file(
+        "test/BUILD",
+        "load(':lib.bzl', 'normal_lib', 'uses_irrelevant')",
+        "uses_irrelevant(name='top', deps=[':shared'])",
+        "normal_lib(name='shared')");
+    useConfiguration(
+        "--experimental_multi_cpu=k8,ppc",
+        "--definitely_relevant=Testing",
+        "--probably_irrelevant=Test 1");
+    update("//test:top");
+    useConfiguration(
+        "--experimental_multi_cpu=k8,ppc",
+        "--definitely_relevant=Testing",
+        "--probably_irrelevant=Test 2");
+    update("//test:top");
+    // the shared library got to reuse the cached value, while the entry point had to be rebuilt in
+    // the new configurations
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 2)
+            .put("//test:shared", 0)
+            .build());
+    useConfiguration(
+        "--experimental_multi_cpu=k8,ppc",
+        "--definitely_relevant=Testing",
+        "--probably_irrelevant=Test 1");
+    update("//test:top");
+    // now we're back to the old configurations with no cache clears, so no work needed to be done
+    assertNumberOfAnalyzedConfigurationsOfTargets(
+        ImmutableMap.<String, Integer>builder()
+            .put("//test:top", 0)
+            .put("//test:shared", 0)
+            .build());
+  }
 }