PlatformMappingValue#map returns BuildConfigurationKey instead of just BuildOptions.

The lone caller needs a `BuildConfgurationKey`, so might as well use the cache to spare the cost of interning it.

PiperOrigin-RevId: 673379910
Change-Id: I90e4e8823fcab0cbca9f3f71f710c82bc52d0615

Author: justinhorvitz

src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java

@@ -140,8 +140,8 @@ private StateMachine applyPlatformMapping(Tasks tasks) {
       return DONE; // Error.
     }
     try {
-      BuildOptions updatedOptions = platformMappingValue.map(options);
-      return finishConfigurationKeyProcessing(BuildConfigurationKey.create(updatedOptions));
+      BuildConfigurationKey updatedKey = platformMappingValue.map(options);
+      return finishConfigurationKeyProcessing(updatedKey);
     } catch (OptionsParsingException e) {
       sink.acceptOptionsParsingError(e);
       return runAfter;

src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValue.java

@@ -31,11 +31,11 @@
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.util.HashCodes;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.concurrent.CompletionException;
 
 /**
@@ -52,7 +52,7 @@ public final class PlatformMappingValue implements SkyValue {
   private final ImmutableMap<Label, ParsedFlagsValue> platformsToFlags;
   private final ImmutableMap<ParsedFlagsValue, Label> flagsToPlatforms;
   private final ImmutableSet<Class<? extends FragmentOptions>> optionsClasses;
-  private final LoadingCache<BuildOptions, BuildOptions> mappingCache;
+  private final LoadingCache<BuildOptions, BuildConfigurationKey> mappingCache;
 
   /**
    * Creates a new mapping value which will match on the given platforms (if a target platform is
@@ -75,7 +75,12 @@ public final class PlatformMappingValue implements SkyValue {
   }
 
   /**
-   * Maps one {@link BuildOptions} to another by way of mappings provided in a file.
+   * Maps one {@link BuildOptions} to another's {@link BuildConfigurationKey} by way of mappings
+   * provided in a file.
+   *
+   * <p>Returns a {@link BuildConfigurationKey} instead of just {@link BuildOptions} so that caching
+   * of mappings also saves the CPU cost of interning {@link BuildConfigurationKey} (which is what
+   * callers typically need).
    *
    * <p>The <a href=https://docs.google.com/document/d/1Vg_tPgiZbSrvXcJ403vZVAGlsWhH9BUDrAxMOYnO0Ls>
    * full design</a> contains the details for the mapping logic but in short:
@@ -89,12 +94,12 @@ public final class PlatformMappingValue implements SkyValue {
    * </ol>
    *
    * @param original the key representing the configuration to be mapped
-   * @return the mapped key if any mapping matched the original or else the original
+   * @return a {@link BuildConfigurationKey} to request the mapped configuration
    * @throws OptionsParsingException if any of the user configured flags cannot be parsed
    * @throws IllegalArgumentException if the original does not contain a {@link PlatformOptions}
    *     fragment
    */
-  public BuildOptions map(BuildOptions original) throws OptionsParsingException {
+  public BuildConfigurationKey map(BuildOptions original) throws OptionsParsingException {
     try {
       return mappingCache.get(original);
     } catch (CompletionException e) {
@@ -104,19 +109,18 @@ public BuildOptions map(BuildOptions original) throws OptionsParsingException {
     }
   }
 
-  private BuildOptions computeMapping(BuildOptions originalOptions) throws OptionsParsingException {
+  private BuildConfigurationKey computeMapping(BuildOptions originalOptions)
+      throws OptionsParsingException {
     if (originalOptions.hasNoConfig()) {
       // The empty configuration (produced by NoConfigTransition) is terminal: it'll never change.
-      return originalOptions;
+      return BuildConfigurationKey.create(originalOptions);
     }
 
     var platformOptions = originalOptions.get(PlatformOptions.class);
     checkArgument(
         platformOptions != null,
         "When using platform mappings, all configurations must contain platform options");
 
-    BuildOptions modifiedOptions = null;
-
     if (!platformOptions.platforms.isEmpty()) {
       List<Label> platforms = platformOptions.platforms;
 
@@ -125,32 +129,28 @@ private BuildOptions computeMapping(BuildOptions originalOptions) throws Options
       if (!platformsToFlags.containsKey(targetPlatform)) {
         // This can happen if the user has set the platform and any other flags that would normally
         // be mapped from it on the command line instead of relying on the mapping.
-        return originalOptions;
+        return BuildConfigurationKey.create(originalOptions);
       }
 
       ParsedFlagsValue parsedFlags = platformsToFlags.get(targetPlatform);
-      modifiedOptions = parsedFlags.mergeWith(originalOptions);
-    } else {
-      boolean mappingFound = false;
-      for (Map.Entry<ParsedFlagsValue, Label> flagsToPlatform : flagsToPlatforms.entrySet()) {
-        ParsedFlagsValue parsedFlags = flagsToPlatform.getKey();
-        Label platformLabel = flagsToPlatform.getValue();
-        if (originalOptions.matches(parsedFlags.parsingResult())) {
-          modifiedOptions = originalOptions.clone();
-          modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(platformLabel);
-          mappingFound = true;
-          break;
-        }
-      }
+      return BuildConfigurationKey.create(parsedFlags.mergeWith(originalOptions));
+    }
 
-      if (!mappingFound) {
-        Label targetPlatform = platformOptions.computeTargetPlatform();
-        modifiedOptions = originalOptions.clone();
-        modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(targetPlatform);
+    for (Map.Entry<ParsedFlagsValue, Label> flagsToPlatform : flagsToPlatforms.entrySet()) {
+      ParsedFlagsValue parsedFlags = flagsToPlatform.getKey();
+      Label platformLabel = flagsToPlatform.getValue();
+      if (originalOptions.matches(parsedFlags.parsingResult())) {
+        BuildOptions modifiedOptions = originalOptions.clone();
+        modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(platformLabel);
+        return BuildConfigurationKey.create(modifiedOptions);
       }
     }
 
-    return modifiedOptions;
+    // No mapping found.
+    Label targetPlatform = platformOptions.computeTargetPlatform();
+    BuildOptions modifiedOptions = originalOptions.clone();
+    modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(targetPlatform);
+    return BuildConfigurationKey.create(modifiedOptions);
   }
 
   @Override
@@ -168,7 +168,7 @@ public boolean equals(Object obj) {
 
   @Override
   public int hashCode() {
-    return Objects.hash(flagsToPlatforms, platformsToFlags, optionsClasses);
+    return HashCodes.hashObjects(flagsToPlatforms, platformsToFlags, optionsClasses);
   }
 
   @Override

src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java

@@ -125,7 +125,7 @@ public void invalidMappingFile_doesNotExist_customLocation() {
   public void invalidMappingFile_doesNotExist_defaultLocation() throws Exception {
     PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingKey.DEFAULT);
 
-    BuildOptions mapped = platformMappingValue.map(createBuildOptions());
+    BuildOptions mapped = platformMappingValue.map(createBuildOptions()).getOptions();
 
     assertThat(mapped.get(PlatformOptions.class).platforms)
         .containsExactly(Label.parseCanonicalUnchecked("@bazel_tools//tools:host_platform"));
@@ -161,7 +161,7 @@ public void mapFromPlatform() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(DummyTestOptions.class).strOption).isEqualTo("one");
   }
@@ -185,7 +185,7 @@ public void mapFromPlatform_fromAlternatePackagePath() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(DummyTestOptions.class).strOption).isEqualTo("one");
   }
@@ -213,7 +213,7 @@ public void mapFromPlatform_noWorkspace() throws Exception {
             PlatformMappingKey.createExplicitlySet(PathFragment.create("my_mapping_file")));
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(DummyTestOptions.class).strOption).isEqualTo("one");
   }
@@ -236,7 +236,7 @@ public void multiplePackagePaths() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(DummyTestOptions.class).strOption).isEqualTo("one");
   }
@@ -266,7 +266,7 @@ public void multiplePackagePathsFirstWins() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(DummyTestOptions.class).strOption).isEqualTo("one");
   }
@@ -289,7 +289,7 @@ public void mapFromPlatform_internalOption() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(DummyTestOptions.class).internalOption).isEqualTo("something_new");
   }
@@ -311,7 +311,7 @@ public void mapFromPlatform_starlarkFlag() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.getStarlarkOptions())
         .containsExactly(Label.parseCanonical("//flag:my_string_flag"), "mapped_value");
@@ -334,7 +334,7 @@ public void mapFromPlatform_listFlag_overridesConfig() throws Exception {
     BuildOptions modifiedOptions =
         createBuildOptions("--platforms=//platforms:one", "--list=from_config");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     // The mapping should completely replace the list, because it is not accumulating.
     assertThat(mapped.get(DummyTestOptions.class).list).containsExactly("from_mapping");
@@ -456,7 +456,7 @@ public void mapFromFlag() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--str_option=one");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM1);
   }
@@ -478,7 +478,7 @@ public void mapFromFlag_starlarkFlag() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--//flag:my_string_flag=mapped_value");
 
-    BuildOptions mapped = platformMappingValue.map(modifiedOptions);
+    BuildOptions mapped = platformMappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM1);
   }

src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java

@@ -147,7 +147,7 @@ public static void computeDefaultPlatformOptions() {
   public void map_noMappings() throws OptionsParsingException {
     PlatformMappingValue mappingValue = builder().build();
 
-    BuildOptions mapped = mappingValue.map(createBuildOptions());
+    BuildOptions mapped = mappingValue.map(createBuildOptions()).getOptions();
 
     assertThat(mapped.get(PlatformOptions.class).platforms)
         .containsExactly(DEFAULT_TARGET_PLATFORM);
@@ -160,7 +160,7 @@ public void map_platformToFlags() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one");
 
-    BuildOptions mapped = mappingValue.map(modifiedOptions);
+    BuildOptions mapped = mappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(DummyTestOptions.class).strOption).isEqualTo("one");
   }
@@ -171,7 +171,7 @@ public void map_flagsToPlatform() throws Exception {
         builder().addFlags(PLATFORM_ONE, "--str_option=one", "--other_str_option=dbg").build();
 
     BuildOptions modifiedOptions = createBuildOptions("--str_option=one", "--other_str_option=dbg");
-    BuildOptions mapped = mappingValue.map(modifiedOptions);
+    BuildOptions mapped = mappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM_ONE);
   }
@@ -186,7 +186,7 @@ public void map_flagsToPlatform_checkPriority() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--str_option=two");
 
-    BuildOptions mapped = mappingValue.map(modifiedOptions);
+    BuildOptions mapped = mappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM_TWO);
   }
@@ -198,7 +198,7 @@ public void map_flagsToPlatform_noneMatching() throws Exception {
 
     BuildOptions modifiedOptions = createBuildOptions("--str_option=bar");
 
-    BuildOptions mapped = mappingValue.map(modifiedOptions);
+    BuildOptions mapped = mappingValue.map(modifiedOptions).getOptions();
 
     assertThat(mapped.get(PlatformOptions.class).platforms)
         .containsExactly(DEFAULT_TARGET_PLATFORM);
@@ -223,7 +223,7 @@ public void map_noMappingIfPlatformIsSetButNotMatching() throws Exception {
 
     BuildOptions modifiedOptions =
         createBuildOptions("--str_option=one", "--platforms=//platforms:two");
-    BuildOptions mapped = mappingValue.map(modifiedOptions);
+    BuildOptions mapped = mappingValue.map(modifiedOptions).getOptions();
 
     // No change because the platform is not in the mapping.
     assertThat(modifiedOptions).isEqualTo(mapped);
@@ -240,7 +240,7 @@ public void map_noMappingIfPlatformIsSetAndNoPlatformMapping() throws Exception
     BuildOptions modifiedOptions =
         createBuildOptions("--str_option=one", "--platforms=//platforms:two");
 
-    BuildOptions mapped = mappingValue.map(modifiedOptions);
+    BuildOptions mapped = mappingValue.map(modifiedOptions).getOptions();
 
     // No change because the platform is not in the mapping.
     assertThat(modifiedOptions).isEqualTo(mapped);