Reduce garbage in options transitions: avoid cloning `BuildOptions` if possible in `AppleCrosstoolTransition` and avoid unnecessary string concatenation in `BuildOptions#get`.
PiperOrigin-RevId: 361568642
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
index 99a6d29..d218edb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -178,7 +178,7 @@
/** Returns the actual instance of a FragmentOptions class. */
public <T extends FragmentOptions> T get(Class<T> optionsClass) {
FragmentOptions options = fragmentOptionsMap.get(optionsClass);
- Preconditions.checkNotNull(options, "fragment options unavailable: " + optionsClass.getName());
+ Preconditions.checkNotNull(options, "fragment options unavailable: %s", optionsClass);
return optionsClass.cast(options);
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleCrosstoolTransition.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleCrosstoolTransition.java
index 40408cc..e60068f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleCrosstoolTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleCrosstoolTransition.java
@@ -23,25 +23,26 @@
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
-import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions;
import com.google.devtools.build.lib.rules.apple.AppleConfiguration;
import com.google.devtools.build.lib.rules.apple.AppleConfiguration.ConfigurationDistinguisher;
import com.google.devtools.build.lib.rules.apple.ApplePlatform;
import com.google.devtools.build.lib.rules.cpp.CppOptions;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
/**
- * Transition that produces a configuration that causes c++ toolchain selection to use the
- * CROSSTOOL given in apple_crosstool_top.
+ * Transition that produces a configuration that causes c++ toolchain selection to use the CROSSTOOL
+ * given in apple_crosstool_top.
*/
-public class AppleCrosstoolTransition implements PatchTransition {
+public final class AppleCrosstoolTransition implements PatchTransition {
/** A singleton instance of AppleCrosstoolTransition. */
- @AutoCodec
+ @SerializationConstant
public static final PatchTransition APPLE_CROSSTOOL_TRANSITION = new AppleCrosstoolTransition();
+ private AppleCrosstoolTransition() {}
+
@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
return ImmutableSet.of(
@@ -50,10 +51,7 @@
@Override
public BuildOptions patch(BuildOptionsView buildOptions, EventHandler eventHandler) {
- BuildOptionsView result = buildOptions.clone();
-
AppleCommandLineOptions appleOptions = buildOptions.get(AppleCommandLineOptions.class);
- CoreOptions configOptions = buildOptions.get(CoreOptions.class);
if (appleOptions.configurationDistinguisher != ConfigurationDistinguisher.UNKNOWN) {
// The configuration distinguisher is only set by AppleCrosstoolTransition and
@@ -62,10 +60,19 @@
return buildOptions.underlying();
}
+ CoreOptions configOptions = buildOptions.get(CoreOptions.class);
String cpu =
ApplePlatform.cpuStringForTarget(
appleOptions.applePlatformType,
determineSingleArchitecture(appleOptions, configOptions));
+
+ // Avoid a clone if nothing would change.
+ if (configOptions.cpu.equals(cpu)
+ && buildOptions.get(CppOptions.class).crosstoolTop.equals(appleOptions.appleCrosstoolTop)) {
+ return buildOptions.underlying();
+ }
+
+ BuildOptionsView result = buildOptions.clone();
setAppleCrosstoolTransitionConfiguration(buildOptions, result, cpu);
return result.underlying();
}
@@ -81,14 +88,13 @@
*/
public static void setAppleCrosstoolTransitionConfiguration(
BuildOptionsView from, BuildOptionsView to, String cpu) {
- Label crosstoolTop = from.get(AppleCommandLineOptions.class).appleCrosstoolTop;
- Label libcTop = from.get(AppleCommandLineOptions.class).appleLibcTop;
- String cppCompiler = from.get(AppleCommandLineOptions.class).cppCompiler;
+ AppleCommandLineOptions appleOptions = from.get(AppleCommandLineOptions.class);
CoreOptions toOptions = to.get(CoreOptions.class);
CppOptions toCppOptions = to.get(CppOptions.class);
- if (toOptions.cpu.equals(cpu) && toCppOptions.crosstoolTop.equals(crosstoolTop)) {
+ if (toOptions.cpu.equals(cpu)
+ && toCppOptions.crosstoolTop.equals(appleOptions.appleCrosstoolTop)) {
// If neither the CPU nor the Crosstool changes, do nothing. This is so that C++ to
// Objective-C dependencies work if the top-level configuration is already an Apple one.
// Removing the configuration distinguisher (which can't be set from the command line) and
@@ -99,14 +105,15 @@
}
toOptions.cpu = cpu;
- toCppOptions.crosstoolTop = crosstoolTop;
+ toCppOptions.crosstoolTop = appleOptions.appleCrosstoolTop;
to.get(AppleCommandLineOptions.class).configurationDistinguisher =
ConfigurationDistinguisher.APPLE_CROSSTOOL;
- to.get(CppOptions.class).cppCompiler = cppCompiler;
- to.get(CppOptions.class).libcTopLabel = libcTop;
+
+ toCppOptions.cppCompiler = appleOptions.cppCompiler;
+ toCppOptions.libcTopLabel = appleOptions.appleLibcTop;
// OSX toolchains do not support fission.
- to.get(CppOptions.class).fissionModes = ImmutableList.of();
+ toCppOptions.fissionModes = ImmutableList.of();
// Ensure platforms aren't set so that platform mapping can take place.
to.get(PlatformOptions.class).platforms = ImmutableList.of();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD b/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD
index 3d2411f..b89c883 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD
@@ -49,7 +49,6 @@
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
- "//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/rules/apple",
"//src/main/java/com/google/devtools/build/lib/rules/apple/cpp",
"//src/main/java/com/google/devtools/build/lib/rules/apple/swift",
@@ -60,6 +59,7 @@
"//src/main/java/com/google/devtools/build/lib/shell",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
+ "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple",
"//src/main/java/com/google/devtools/build/lib/util",
