Remove `OptionsDiffForReconstruction` in favor of just using `BuildOptions`. Now that all `BuildOptions` serialization is done via checksum, there is no need for `OptionsDiffForReconstruction`, which introduces a lot of code and some overhead even though most builds don't do any serialization. I will kick off some benchmarks, but my preliminary analysis suggests that there's no significant memory benefit in storing just the diff in `BuildConfigurationValue.Key`. PiperOrigin-RevId: 370159473
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java index 735276c..ee8a937b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java
@@ -41,9 +41,7 @@ String FILES_FIELD = "files"; default String getConfigurationChecksum() { - return getConfigurationKey() == null - ? null - : getConfigurationKey().getOptionsDiff().getChecksum(); + return getConfigurationKey() == null ? null : getConfigurationKey().getOptions().checksum(); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index fbd8e11..8f306bf 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -107,7 +107,7 @@ private final ImmutableMap<String, Class<? extends Fragment>> starlarkVisibleFragments; private final RepositoryName mainRepositoryName; private final ImmutableSet<String> reservedActionMnemonics; - private CommandLineLimits commandLineLimits; + private final CommandLineLimits commandLineLimits; /** * The global "make variables" such as "$(TARGET_CPU)"; these get applied to all rules analyzed in @@ -119,12 +119,10 @@ private final ActionEnvironment testEnv; private final BuildOptions buildOptions; - private final BuildOptions.OptionsDiffForReconstruction buildOptionsDiff; private final CoreOptions options; private final ImmutableMap<String, String> commandLineBuildVariables; - private final String checksum; private final int hashCode; // We can precompute the hash code as all its inputs are immutable. /** Data for introspecting the options used by this configuration. */ @@ -227,7 +225,6 @@ BlazeDirectories directories, Map<Class<? extends Fragment>, Fragment> fragmentsMap, BuildOptions buildOptions, - BuildOptions.OptionsDiffForReconstruction buildOptionsDiff, ImmutableSet<String> reservedActionMnemonics, ActionEnvironment actionEnvironment, String repositoryName, @@ -237,7 +234,6 @@ directories, fragmentsMap, buildOptions, - buildOptionsDiff, reservedActionMnemonics, actionEnvironment, RepositoryName.createFromValidStrippedName(repositoryName), @@ -250,18 +246,15 @@ BlazeDirectories directories, Map<Class<? extends Fragment>, Fragment> fragmentsMap, BuildOptions buildOptions, - BuildOptions.OptionsDiffForReconstruction buildOptionsDiff, ImmutableSet<String> reservedActionMnemonics, ActionEnvironment actionEnvironment, RepositoryName mainRepositoryName, boolean siblingRepositoryLayout) throws InvalidMnemonicException { - // this.directories = directories; this.fragments = makeFragmentsMap(fragmentsMap); this.fragmentClassSet = FragmentClassSet.of(this.fragments.keySet()); this.starlarkVisibleFragments = buildIndexOfStarlarkVisibleFragments(); - this.buildOptions = buildOptions.clone(); - this.buildOptionsDiff = buildOptionsDiff; + this.buildOptions = buildOptions; this.options = buildOptions.get(CoreOptions.class); this.outputDirectories = new OutputDirectories( @@ -302,7 +295,6 @@ "GENDIR", getGenfilesDirectory(RepositoryName.MAIN).getExecPath().getPathString()); globalMakeEnv = globalMakeEnvBuilder.build(); - checksum = buildOptions.computeChecksum(); hashCode = computeHashCode(); this.reservedActionMnemonics = reservedActionMnemonics; @@ -315,10 +307,7 @@ * configuration is assumed to have). */ public BuildConfiguration clone( - FragmentClassSet fragmentClasses, - RuleClassProvider ruleClassProvider, - BuildOptions defaultBuildOptions) { - + FragmentClassSet fragmentClasses, RuleClassProvider ruleClassProvider) { ClassToInstanceMap<Fragment> fragmentsMap = MutableClassToInstanceMap.create(); for (Fragment fragment : fragments.values()) { if (fragmentClasses.fragmentClasses().contains(fragment.getClass())) { @@ -332,7 +321,6 @@ getDirectories(), fragmentsMap, options, - BuildOptions.diffForReconstruction(defaultBuildOptions, options), reservedActionMnemonics, actionEnv, mainRepositoryName.strippedName(), @@ -794,13 +782,12 @@ /** Returns the cache key of the build options used to create this configuration. */ public String checksum() { - return checksum; + return buildOptions.checksum(); } /** Returns a copy of the build configuration options for this configuration. */ public BuildOptions cloneOptions() { - BuildOptions clone = buildOptions.clone(); - return clone; + return buildOptions.clone(); } /** @@ -817,10 +804,6 @@ return buildOptions; } - public BuildOptions.OptionsDiffForReconstruction getBuildOptionsDiff() { - return buildOptionsDiff; - } - public String getCpu() { return options.cpu; }
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 bb123e8..e4d4d0e 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
@@ -14,18 +14,16 @@ package com.google.devtools.build.lib.analysis.config; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.MoreObjects; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.MapDifference; import com.google.common.collect.Maps; @@ -36,7 +34,6 @@ import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.OrderedSetMultimap; import com.google.devtools.common.options.OptionDefinition; @@ -50,10 +47,7 @@ import com.google.protobuf.CodedOutputStream; import java.io.IOException; import java.io.Serializable; -import java.lang.ref.SoftReference; import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -64,14 +58,12 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.ExecutionException; import java.util.stream.Collectors; import javax.annotation.Nullable; /** Stores the command-line options from a set of configuration fragments. */ // TODO(janakr): If overhead of FragmentOptions class names is too high, add constructor that just // takes fragments and gets names from them. -@AutoCodec public final class BuildOptions implements Cloneable, Serializable { private static final Comparator<Class<? extends FragmentOptions>> lexicalFragmentOptionsComparator = Comparator.comparing(Class::getName); @@ -152,9 +144,7 @@ throws OptionsParsingException { Builder builder = builder(); OptionsParser parser = - OptionsParser.builder() - .optionsClasses(ImmutableList.<Class<? extends OptionsBase>>copyOf(optionsList)) - .build(); + OptionsParser.builder().optionsClasses(ImmutableList.copyOf(optionsList)).build(); parser.parse(args); for (Class<? extends FragmentOptions> optionsClass : optionsList) { builder.addFragmentOptions(parser.getOptions(optionsClass)); @@ -182,21 +172,21 @@ return fragmentOptionsMap.containsKey(optionsClass); } - /** The cache key for the options collection. Recomputes cache key every time it's called. */ - public String computeCacheKey() { - StringBuilder keyBuilder = new StringBuilder(); - for (FragmentOptions options : fragmentOptionsMap.values()) { - keyBuilder.append(options.cacheKey()); + /** Returns a hex digest string uniquely identifying the build options. */ + public String checksum() { + if (checksum == null) { + synchronized (this) { + if (checksum == null) { + Fingerprint fingerprint = new Fingerprint(); + for (FragmentOptions options : fragmentOptionsMap.values()) { + fingerprint.addString(options.cacheKey()); + } + fingerprint.addString(OptionsBase.mapToCacheKey(starlarkOptionsMap)); + checksum = fingerprint.hexDigestAndReset(); + } + } } - keyBuilder.append( - OptionsBase.mapToCacheKey( - starlarkOptionsMap.entrySet().stream() - .collect(Collectors.toMap(e -> e.getKey().toString(), Map.Entry::getValue)))); - return keyBuilder.toString(); - } - - public String computeChecksum() { - return Fingerprint.getHexDigest(computeCacheKey()); + return checksum; } /** String representation of build options. */ @@ -210,12 +200,12 @@ } /** Returns the options contained in this collection. */ - public Collection<FragmentOptions> getNativeOptions() { + public ImmutableCollection<FragmentOptions> getNativeOptions() { return fragmentOptionsMap.values(); } /** Returns the set of fragment classes contained in these options. */ - public Set<Class<? extends FragmentOptions>> getFragmentClasses() { + public ImmutableSet<Class<? extends FragmentOptions>> getFragmentClasses() { return fragmentOptionsMap.keySet(); } @@ -230,7 +220,7 @@ @Override public BuildOptions clone() { ImmutableMap.Builder<Class<? extends FragmentOptions>, FragmentOptions> nativeOptionsBuilder = - ImmutableMap.builder(); + ImmutableMap.builderWithExpectedSize(fragmentOptionsMap.size()); for (Map.Entry<Class<? extends FragmentOptions>, FragmentOptions> entry : fragmentOptionsMap.entrySet()) { nativeOptionsBuilder.put(entry.getKey(), entry.getValue().clone()); @@ -238,120 +228,40 @@ return new BuildOptions(nativeOptionsBuilder.build(), ImmutableMap.copyOf(starlarkOptionsMap)); } - private boolean fingerprintAndHashCodeInitialized() { - return fingerprint != null; - } - - /** - * Lazily initialize {@link #fingerprint} and {@link #hashCode}. Keeps computation off critical - * path of build, while still avoiding expensive computation for equality and hash code each time. - * - * <p>We check {@link #fingerprintAndHashCodeInitialized} to see if this method has already been - * called. Using {@link #hashCode} after this method is called is safe because it is set here - * before {@link #fingerprint} is set, so if {@link #fingerprint} is non-null then {@link - * #hashCode} is definitely set. - */ - private void maybeInitializeFingerprintAndHashCode() { - if (fingerprintAndHashCodeInitialized()) { - return; - } - synchronized (this) { - if (fingerprintAndHashCodeInitialized()) { - return; - } - Fingerprint fingerprint = new Fingerprint(); - for (Map.Entry<Class<? extends FragmentOptions>, FragmentOptions> entry : - fragmentOptionsMap.entrySet()) { - fingerprint.addString(entry.getKey().getName()); - fingerprint.addString(entry.getValue().cacheKey()); - } - for (Map.Entry<Label, Object> entry : starlarkOptionsMap.entrySet()) { - fingerprint.addString(entry.getKey().toString()); - fingerprint.addString(entry.getValue().toString()); - } - byte[] computedFingerprint = fingerprint.digestAndReset(); - hashCode = Arrays.hashCode(computedFingerprint); - this.fingerprint = computedFingerprint; - } - } - @Override public boolean equals(Object other) { if (this == other) { return true; - } else if (!(other instanceof BuildOptions)) { - return false; - } else { - maybeInitializeFingerprintAndHashCode(); - BuildOptions otherOptions = (BuildOptions) other; - otherOptions.maybeInitializeFingerprintAndHashCode(); - return Arrays.equals(this.fingerprint, otherOptions.fingerprint); } + if (!(other instanceof BuildOptions)) { + return false; + } + return checksum().equals(((BuildOptions) other).checksum()); } @Override public int hashCode() { - maybeInitializeFingerprintAndHashCode(); - return hashCode; + return 31 + checksum().hashCode(); } - // Lazily initialized. - @Nullable private volatile byte[] fingerprint; - private volatile int hashCode; - /** Maps options class definitions to FragmentOptions objects. */ private final ImmutableMap<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptionsMap; /** Maps Starlark options names to Starlark options values. */ private final ImmutableMap<Label, Object> starlarkOptionsMap; - @AutoCodec.VisibleForSerialization - BuildOptions( + // Lazily initialized both for performance and correctness - BuildOptions instances may be mutated + // after construction but before consumption. Access via checksum() to ensure initialization. This + // field is volatile as per https://errorprone.info/bugpattern/DoubleCheckedLocking, which + // encourages using volatile even for immutable objects. + @Nullable private volatile String checksum = null; + + private BuildOptions( ImmutableMap<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptionsMap, ImmutableMap<Label, Object> starlarkOptionsMap) { this.fragmentOptionsMap = fragmentOptionsMap; this.starlarkOptionsMap = starlarkOptionsMap; } - public BuildOptions applyDiff(OptionsDiffForReconstruction optionsDiff) { - if (optionsDiff.isEmpty()) { - return this; - } - maybeInitializeFingerprintAndHashCode(); - if (!Arrays.equals(fingerprint, optionsDiff.baseFingerprint)) { - throw new IllegalArgumentException("Cannot reconstruct BuildOptions with a different base."); - } - BuildOptions reconstructedOptions = optionsDiff.cachedReconstructed.get(); - if (reconstructedOptions != null) { - return reconstructedOptions; - } - Builder builder = builder(); - for (FragmentOptions options : fragmentOptionsMap.values()) { - FragmentOptions newOptions = optionsDiff.transformOptions(options); - if (newOptions != null) { - builder.addFragmentOptions(newOptions); - } - } - for (FragmentOptions extraSecondFragment : optionsDiff.extraSecondFragments) { - builder.addFragmentOptions(extraSecondFragment); - } - - Map<Label, Object> starlarkOptions = new HashMap<>(); - for (Map.Entry<Label, Object> buildSettingAndValue : starlarkOptionsMap.entrySet()) { - Label buildSetting = buildSettingAndValue.getKey(); - if (optionsDiff.extraFirstStarlarkOptions.contains(buildSetting)) { - continue; - } else if (optionsDiff.differingStarlarkOptions.containsKey(buildSetting)) { - starlarkOptions.put(buildSetting, optionsDiff.differingStarlarkOptions.get(buildSetting)); - } else { - starlarkOptions.put(buildSetting, starlarkOptionsMap.get(buildSetting)); - } - } - starlarkOptions.putAll(optionsDiff.extraSecondStarlarkOptions); - reconstructedOptions = builder.addStarlarkOptions(starlarkOptions).build(); - optionsDiff.cachedReconstructed = new SoftReference<>(reconstructedOptions); - return reconstructedOptions; - } - /** * Applies any options set in the parsing result on top of these options, returning the resulting * build options. @@ -407,9 +317,7 @@ continue; } FragmentOptions newOptions = - replacedOptions.computeIfAbsent( - fragmentOptionClass, - (Class<? extends FragmentOptions> k) -> originalFragment.clone()); + replacedOptions.computeIfAbsent(fragmentOptionClass, unused -> originalFragment.clone()); try { Object value = parsingResult.getOptionValueDescription(optionDefinition.getOptionName()).getValue(); @@ -570,25 +478,19 @@ * aggregating the difference between a single BuildOptions and the results of applying a {@link * com.google.devtools.build.lib.analysis.config.transitions.SplitTransition}) to it. */ - @SuppressWarnings("ReferenceEquality") // See comments above == comparisons. + @SuppressWarnings("ReferenceEquality") // See comment above == comparison. public static OptionsDiff diff(OptionsDiff diff, BuildOptions first, BuildOptions second) { - if (diff.hasStarlarkOptions) { - throw new IllegalStateException( - "OptionsDiff cannot handle multiple 'second' BuildOptions with Starlark options " - + "and is trying to diff against a second BuildOptions with Starlark options."); - } - if (first == null || second == null) { - throw new IllegalArgumentException("Cannot diff null BuildOptions"); - } - // For performance reasons, we avoid calling #equals unless both instances have had their - // fingerprint and hash code initialized. We don't typically encounter value-equal instances - // here anyway. - if (first == second - || (first.fingerprintAndHashCodeInitialized() - && second.fingerprintAndHashCodeInitialized() - && first.equals(second))) { + checkArgument( + !diff.hasStarlarkOptions, + "OptionsDiff cannot handle multiple 'second' BuildOptions with Starlark options and is" + + " trying to diff against %s", + diff); + checkNotNull(first); + checkNotNull(second); + if (first.equals(second)) { return diff; } + // Check and report if either class has been trimmed of an options class that exists in the // other. ImmutableSet<Class<? extends FragmentOptions>> firstOptionClasses = @@ -609,8 +511,7 @@ Sets.intersection(firstOptionClasses, secondOptionClasses)) { FragmentOptions firstOptions = first.get(clazz); FragmentOptions secondOptions = second.get(clazz); - // Similar to above, we avoid calling #equals because we are going to do a field-by-field - // comparison anyway. + // We avoid calling #equals because we are going to do a field-by-field comparison anyway. if (firstOptions == secondOptions) { continue; } @@ -640,94 +541,10 @@ } /** - * Cache for {@link OptionsDiffForReconstruction}, which is expensive to compute. - * - * <p>The reason for using {@linkplain CacheBuilder#weakKeys weak keys} is twofold: we want - * objects in the cache to be garbage collected, and we also want to use reference equality to - * avoid the expensive initialization in {@link #maybeInitializeFingerprintAndHashCode}. - */ - private static final Cache<BuildOptions, OptionsDiffForReconstruction> - diffForReconstructionCache = CacheBuilder.newBuilder().weakKeys().build(); - - /** - * Returns a {@link OptionsDiffForReconstruction} object that can be applied to {@code first} via - * {@link #applyDiff} to get a {@link BuildOptions} object equal to {@code second}. - */ - public static OptionsDiffForReconstruction diffForReconstruction( - BuildOptions first, BuildOptions second) { - OptionsDiffForReconstruction diff; - try { - diff = - diffForReconstructionCache.get(second, () -> createDiffForReconstruction(first, second)); - } catch (ExecutionException e) { - throw new IllegalStateException(e); - } - - // We need to ensure that the possibly cached diff was computed against the same base options. - // In practice this should always be the case, since callers pass in a "default" options - // instance as "first". To be safe however, we create an uncached diff if there is a mismatch. - // Note that this check should be fast because the fingerprints should be reference-equal. - return Arrays.equals(first.fingerprint, diff.baseFingerprint) - ? diff - : createDiffForReconstruction(first, second); - } - - private static OptionsDiffForReconstruction createDiffForReconstruction( - BuildOptions first, BuildOptions second) { - OptionsDiff diff = diff(first, second); - if (diff.areSame()) { - first.maybeInitializeFingerprintAndHashCode(); - return OptionsDiffForReconstruction.getEmpty(first.fingerprint, second.computeChecksum()); - } - LinkedHashMap<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions = - new LinkedHashMap<>(diff.differingOptions.keySet().size()); - for (Class<? extends FragmentOptions> clazz : - diff.differingOptions.keySet().stream() - .sorted(lexicalFragmentOptionsComparator) - .collect(Collectors.toList())) { - Collection<OptionDefinition> fields = diff.differingOptions.get(clazz); - LinkedHashMap<String, Object> valueMap = new LinkedHashMap<>(fields.size()); - for (OptionDefinition optionDefinition : - fields.stream() - .sorted(Comparator.comparing(o -> o.getField().getName())) - .collect(Collectors.toList())) { - Object secondValue; - try { - secondValue = Iterables.getOnlyElement(diff.second.get(optionDefinition)); - } catch (IllegalArgumentException e) { - // TODO(janakr): Currently this exception should never be thrown since diff is never - // constructed using the diff method that takes in a preexisting OptionsDiff. If this - // changes, add a test verifying this error catching works properly. - throw new IllegalStateException( - "OptionsDiffForReconstruction can only handle a single first BuildOptions and a " - + "single second BuildOptions and has encountered multiple second BuildOptions", - e); - } - valueMap.put(optionDefinition.getField().getName(), secondValue); - } - differingOptions.put(clazz, valueMap); - } - first.maybeInitializeFingerprintAndHashCode(); - return new OptionsDiffForReconstruction( - differingOptions, - diff.extraFirstFragments.stream() - .sorted(lexicalFragmentOptionsComparator) - .collect(ImmutableSet.toImmutableSet()), - ImmutableList.sortedCopyOf( - Comparator.comparing(o -> o.getClass().getName()), diff.extraSecondFragments), - first.fingerprint, - second.computeChecksum(), - diff.starlarkSecond, - diff.extraStarlarkOptionsFirst, - diff.extraStarlarkOptionsSecond, - second); - } - - /** * A diff class for BuildOptions. Fields are meant to be populated and returned by {@link - * BuildOptions#diff} + * BuildOptions#diff}. */ - public static class OptionsDiff { + public static final class OptionsDiff { private final Multimap<Class<? extends FragmentOptions>, OptionDefinition> differingOptions = ArrayListMultimap.create(); // The keyset for the {@link first} and {@link second} maps are identical and indicate which @@ -871,187 +688,50 @@ } } - /** - * An object that encapsulates the data needed to transform one {@link BuildOptions} object into - * another: the full fragments of the second one, the fragment classes of the first that should be - * omitted, and the values of any fields that should be changed. - */ - public static final class OptionsDiffForReconstruction { - private final Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions; - private final ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses; - private final ImmutableList<FragmentOptions> extraSecondFragments; - private final byte[] baseFingerprint; - private final String checksum; + @SuppressWarnings("unused") // Used reflectively. + private static final class Codec implements ObjectCodec<BuildOptions> { - private final Map<Label, Object> differingStarlarkOptions; - private final List<Label> extraFirstStarlarkOptions; - private final Map<Label, Object> extraSecondStarlarkOptions; - - /** - * A soft reference to the reconstructed build options to save work and garbage creation in - * {@link #applyDiff}. - * - * <p>Promotes reuse of a single {@code BuildOptions} instance to preserve reference equality - * and limit fingerprint/hashCode initialization. - */ - private SoftReference<BuildOptions> cachedReconstructed; - - public OptionsDiffForReconstruction( - Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions, - ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses, - ImmutableList<FragmentOptions> extraSecondFragments, - byte[] baseFingerprint, - String checksum, - Map<Label, Object> differingStarlarkOptions, - List<Label> extraFirstStarlarkOptions, - Map<Label, Object> extraSecondStarlarkOptions, - @Nullable BuildOptions original) { - this.differingOptions = differingOptions; - this.extraFirstFragmentClasses = extraFirstFragmentClasses; - this.extraSecondFragments = extraSecondFragments; - this.baseFingerprint = baseFingerprint; - this.checksum = checksum; - this.differingStarlarkOptions = differingStarlarkOptions; - this.extraFirstStarlarkOptions = extraFirstStarlarkOptions; - this.extraSecondStarlarkOptions = extraSecondStarlarkOptions; - this.cachedReconstructed = new SoftReference<>(original); - } - - private static OptionsDiffForReconstruction getEmpty(byte[] baseFingerprint, String checksum) { - return new OptionsDiffForReconstruction( - ImmutableMap.of(), - ImmutableSet.of(), - ImmutableList.of(), - baseFingerprint, - checksum, - ImmutableMap.of(), - ImmutableList.of(), - ImmutableMap.of(), - /*original=*/ null); - } - - @Nullable - @VisibleForTesting - FragmentOptions transformOptions(FragmentOptions input) { - Class<? extends FragmentOptions> clazz = input.getClass(); - if (extraFirstFragmentClasses.contains(clazz)) { - return null; - } - Map<String, Object> changedOptions = differingOptions.get(clazz); - if (changedOptions == null || changedOptions.isEmpty()) { - return input; - } - FragmentOptions newOptions = input.clone(); - for (Map.Entry<String, Object> entry : changedOptions.entrySet()) { - try { - clazz.getField(entry.getKey()).set(newOptions, entry.getValue()); - } catch (IllegalAccessException | NoSuchFieldException e) { - throw new IllegalStateException("Couldn't set " + entry + " for " + newOptions, e); - } - } - return newOptions; - } - - public String getChecksum() { - return checksum; - } - - private boolean isEmpty() { - return differingOptions.isEmpty() - && extraFirstFragmentClasses.isEmpty() - && extraSecondFragments.isEmpty() - && differingStarlarkOptions.isEmpty() - && extraFirstStarlarkOptions.isEmpty() - && extraSecondStarlarkOptions.isEmpty(); - } - - /** - * Clears {@link #cachedReconstructed} so that tests can cover the core logic of {@link - * #applyDiff}. - */ - @VisibleForTesting - void clearCachedReconstructedForTesting() { - cachedReconstructed = new SoftReference<>(null); + @Override + public Class<BuildOptions> getEncodedClass() { + return BuildOptions.class; } @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (!(o instanceof OptionsDiffForReconstruction)) { - return false; - } - OptionsDiffForReconstruction that = (OptionsDiffForReconstruction) o; - return Arrays.equals(this.baseFingerprint, that.baseFingerprint) - && this.checksum.equals(that.checksum); + public void serialize( + SerializationContext context, BuildOptions options, CodedOutputStream codedOut) + throws IOException { + context.getDependency(OptionsChecksumCache.class).prime(options); + codedOut.writeStringNoTag(options.checksum()); } @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("differingOptions", differingOptions) - .add("extraFirstFragmentClasses", extraFirstFragmentClasses) - .add("extraSecondFragments", extraSecondFragments) - .add("differingStarlarkOptions", differingStarlarkOptions) - .add("extraFirstStarlarkOptions", extraFirstStarlarkOptions) - .add("extraSecondStarlarkOptions", extraSecondStarlarkOptions) - .toString(); - } - - @Override - public int hashCode() { - return 31 * Arrays.hashCode(baseFingerprint) + checksum.hashCode(); - } - - @SuppressWarnings("unused") // Used reflectively. - private static final class Codec implements ObjectCodec<OptionsDiffForReconstruction> { - - @Override - public Class<OptionsDiffForReconstruction> getEncodedClass() { - return OptionsDiffForReconstruction.class; - } - - @Override - public void serialize( - SerializationContext context, - OptionsDiffForReconstruction optionsDiff, - CodedOutputStream codedOut) - throws IOException { - context.getDependency(OptionsChecksumCache.class).prime(optionsDiff); - codedOut.writeStringNoTag(optionsDiff.getChecksum()); - } - - @Override - public OptionsDiffForReconstruction deserialize( - DeserializationContext context, CodedInputStream codedIn) throws IOException { + public BuildOptions deserialize(DeserializationContext context, CodedInputStream codedIn) + throws IOException { String checksum = codedIn.readString(); - return checkNotNull( - context.getDependency(OptionsChecksumCache.class).getOptionsDiff(checksum), - "No options instance for %s", - checksum); - } + return checkNotNull( + context.getDependency(OptionsChecksumCache.class).getOptions(checksum), + "No options instance for %s", + checksum); } } /** - * Provides {@link OptionsDiffForReconstruction} instances when requested via their {@linkplain - * OptionsDiffForReconstruction#getChecksum checksum}. + * Provides {@link BuildOptions} instances when requested via their {@linkplain + * BuildOptions#checksum() checksum}. */ public interface OptionsChecksumCache { /** - * Called during deserialization to transform a checksum into an {@link - * OptionsDiffForReconstruction} instance. + * Called during deserialization to transform a checksum into a {@link BuildOptions} instance. */ - OptionsDiffForReconstruction getOptionsDiff(String checksum); + BuildOptions getOptions(String checksum); /** * Notifies the cache that it may be necessary to deserialize the given options diff's checksum. * - * <p>Called each time an {@link OptionsDiffForReconstruction} instance is serialized. + * <p>Called each time an {@link BuildOptions} instance is serialized. */ - void prime(OptionsDiffForReconstruction optionsDiff); + void prime(BuildOptions options); } /** @@ -1060,17 +740,16 @@ * <p>Checksum mappings are retained indefinitely. */ public static final class MapBackedChecksumCache implements OptionsChecksumCache { - private final ConcurrentMap<String, OptionsDiffForReconstruction> map = - new ConcurrentHashMap<>(); + private final ConcurrentMap<String, BuildOptions> map = new ConcurrentHashMap<>(); @Override - public OptionsDiffForReconstruction getOptionsDiff(String checksum) { + public BuildOptions getOptions(String checksum) { return map.get(checksum); } @Override - public void prime(OptionsDiffForReconstruction optionsDiff) { - map.putIfAbsent(optionsDiff.getChecksum(), optionsDiff); + public void prime(BuildOptions options) { + map.putIfAbsent(options.checksum(), options); } } }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java index 5f67920..63ed49f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -271,10 +271,7 @@ String transitionKey = optionsEntry.getKey(); BuildConfigurationValue.Key buildConfigurationValueKey = BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, - defaultBuildOptions, - depFragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, optionsEntry.getValue())); + platformMappingValue, defaultBuildOptions, depFragments, optionsEntry.getValue()); configurationKeys.put(transitionKey, buildConfigurationValueKey); } } catch (OptionsParsingException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/KeyedConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/KeyedConfiguredTarget.java index 1c741cd..ed56d68 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/KeyedConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/KeyedConfiguredTarget.java
@@ -56,9 +56,7 @@ /** Returns the configuration checksum in use for this KeyedConfiguredTarget. */ @Nullable public String getConfigurationChecksum() { - return getConfigurationKey() == null - ? null - : getConfigurationKey().getOptionsDiff().getChecksum(); + return getConfigurationKey() == null ? null : getConfigurationKey().getOptions().checksum(); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java index 793357f..253d5d7 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java
@@ -175,7 +175,7 @@ toOptions.stream() .map( options -> { - String checksum = options.computeChecksum(); + String checksum = options.checksum(); return checksum.equals(hostConfigurationChecksum) ? "HOST" : shortId(checksum); @@ -193,7 +193,7 @@ } } - private String getRuleClassTransition(ConfiguredTarget ct, Target target) { + private static String getRuleClassTransition(ConfiguredTarget ct, Target target) { String output = ""; if (ct instanceof RuleConfiguredTarget) { TransitionFactory<Rule> factory =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java index be80666..a3d667b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
@@ -42,16 +42,14 @@ import java.util.concurrent.ExecutionException; import net.starlark.java.eval.StarlarkSemantics; -/** - * A builder for {@link BuildConfigurationValue} instances. - */ -public class BuildConfigurationFunction implements SkyFunction { +/** A builder for {@link BuildConfigurationValue} instances. */ +public final class BuildConfigurationFunction implements SkyFunction { + /** Cache with weak values can't have null values. */ private static final Fragment NULL_MARKER = new Fragment() {}; private final BlazeDirectories directories; private final ConfiguredRuleClassProvider ruleClassProvider; - private final BuildOptions defaultBuildOptions; private final LoadingCache<FragmentKey, Fragment> fragmentCache = CacheBuilder.newBuilder() .weakValues() @@ -64,12 +62,9 @@ }); public BuildConfigurationFunction( - BlazeDirectories directories, - RuleClassProvider ruleClassProvider, - BuildOptions defaultBuildOptions) { + BlazeDirectories directories, RuleClassProvider ruleClassProvider) { this.directories = directories; this.ruleClassProvider = (ConfiguredRuleClassProvider) ruleClassProvider; - this.defaultBuildOptions = defaultBuildOptions; } @Override @@ -102,17 +97,15 @@ fragmentsMap.put(fragment.getClass(), fragment); } - BuildOptions options = defaultBuildOptions.applyDiff(key.getOptionsDiff()); ActionEnvironment actionEnvironment = - ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(options); + ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(key.getOptions()); try { return new BuildConfigurationValue( new BuildConfiguration( directories, fragmentsMap, - options, - key.getOptionsDiff(), + key.getOptions(), ruleClassProvider.getReservedActionMnemonics(), actionEnvironment, workspaceNameValue.getName(), @@ -125,15 +118,15 @@ private Set<Fragment> getConfigurationFragments(BuildConfigurationValue.Key key) throws InvalidConfigurationException { - BuildOptions options = defaultBuildOptions.applyDiff(key.getOptionsDiff()); ImmutableSortedSet<Class<? extends Fragment>> fragmentClasses = key.getFragments(); ImmutableSet.Builder<Fragment> fragments = ImmutableSet.builderWithExpectedSize(fragmentClasses.size()); for (Class<? extends Fragment> fragmentClass : fragmentClasses) { BuildOptions trimmedOptions = - options.trim( - BuildConfiguration.getOptionsClasses( - ImmutableList.of(fragmentClass), ruleClassProvider)); + key.getOptions() + .trim( + BuildConfiguration.getOptionsClasses( + ImmutableList.of(fragmentClass), ruleClassProvider)); Fragment fragment; FragmentKey fragmentKey = FragmentKey.create(trimmedOptions, fragmentClass); try { @@ -165,7 +158,8 @@ } } - private Fragment makeFragment(FragmentKey fragmentKey) throws InvalidConfigurationException { + private static Fragment makeFragment(FragmentKey fragmentKey) + throws InvalidConfigurationException { BuildOptions buildOptions = fragmentKey.getBuildOptions(); Class<? extends Fragment> fragmentClass = fragmentKey.getFragmentClass(); String noConstructorPattern = "%s lacks constructor(BuildOptions)"; @@ -189,7 +183,7 @@ } private static final class BuildConfigurationFunctionException extends SkyFunctionException { - public BuildConfigurationFunctionException(InvalidConfigurationException e) { + BuildConfigurationFunctionException(InvalidConfigurationException e) { super(e, Transience.PERSISTENT); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java index dda2e78..1f41abd 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java
@@ -50,7 +50,7 @@ } /** - * Creates a new configuration key based on the given diff, after applying a platform mapping + * Creates a new configuration key based on the given options, after applying a platform mapping * transformation. * * @param platformMappingValue sky value that can transform a configuration key based on a @@ -58,21 +58,21 @@ * @param defaultBuildOptions set of native build options without modifications based on parsing * flags * @param fragments set of options fragments this configuration should cover - * @param optionsDiff diff between the default options and the desired configuration + * @param options the desired configuration * @throws OptionsParsingException if the platform mapping cannot be parsed */ public static Key keyWithPlatformMapping( PlatformMappingValue platformMappingValue, BuildOptions defaultBuildOptions, FragmentClassSet fragments, - BuildOptions.OptionsDiffForReconstruction optionsDiff) + BuildOptions options) throws OptionsParsingException { return platformMappingValue.map( - keyWithoutPlatformMapping(fragments, optionsDiff), defaultBuildOptions); + keyWithoutPlatformMapping(fragments, options), defaultBuildOptions); } /** - * Creates a new configuration key based on the given diff, after applying a platform mapping + * Creates a new configuration key based on the given options, after applying a platform mapping * transformation. * * @param platformMappingValue sky value that can transform a configuration key based on a @@ -80,17 +80,17 @@ * @param defaultBuildOptions set of native build options without modifications based on parsing * flags * @param fragments set of options fragments this configuration should cover - * @param optionsDiff diff between the default options and the desired configuration + * @param options the desired configuration * @throws OptionsParsingException if the platform mapping cannot be parsed */ public static Key keyWithPlatformMapping( PlatformMappingValue platformMappingValue, BuildOptions defaultBuildOptions, Set<Class<? extends Fragment>> fragments, - BuildOptions.OptionsDiffForReconstruction optionsDiff) + BuildOptions options) throws OptionsParsingException { return platformMappingValue.map( - keyWithoutPlatformMapping(fragments, optionsDiff), defaultBuildOptions); + keyWithoutPlatformMapping(fragments, options), defaultBuildOptions); } /** @@ -100,22 +100,20 @@ * mapping is not required. * * @param fragments the fragments the configuration should contain - * @param optionsDiff the {@link BuildOptions.OptionsDiffForReconstruction} object the {@link - * BuildOptions} should be rebuilt from + * @param options the {@link BuildOptions} object the {@link BuildOptions} should be rebuilt from */ @ThreadSafe static Key keyWithoutPlatformMapping( - Set<Class<? extends Fragment>> fragments, - BuildOptions.OptionsDiffForReconstruction optionsDiff) { + Set<Class<? extends Fragment>> fragments, BuildOptions options) { return Key.create( FragmentClassSet.of( ImmutableSortedSet.copyOf(BuildConfiguration.lexicalFragmentSorter, fragments)), - optionsDiff); + options); } private static Key keyWithoutPlatformMapping( - FragmentClassSet fragmentClassSet, BuildOptions.OptionsDiffForReconstruction optionsDiff) { - return Key.create(fragmentClassSet, optionsDiff); + FragmentClassSet fragmentClassSet, BuildOptions options) { + return Key.create(fragmentClassSet, options); } /** @@ -129,7 +127,7 @@ */ public static Key key(BuildConfiguration buildConfiguration) { return keyWithoutPlatformMapping( - buildConfiguration.fragmentClasses(), buildConfiguration.getBuildOptionsDiff()); + buildConfiguration.fragmentClasses(), buildConfiguration.getOptions()); } /** {@link SkyKey} for {@link BuildConfigurationValue}. */ @@ -138,20 +136,19 @@ private static final Interner<Key> keyInterner = BlazeInterners.newWeakInterner(); private final FragmentClassSet fragments; - private final BuildOptions.OptionsDiffForReconstruction optionsDiff; + private final BuildOptions options; private final int hashCode; @AutoCodec.Instantiator @VisibleForSerialization - static Key create( - FragmentClassSet fragments, BuildOptions.OptionsDiffForReconstruction optionsDiff) { - return keyInterner.intern(new Key(fragments, optionsDiff)); + static Key create(FragmentClassSet fragments, BuildOptions options) { + return keyInterner.intern(new Key(fragments, options)); } - private Key(FragmentClassSet fragments, BuildOptions.OptionsDiffForReconstruction optionsDiff) { + private Key(FragmentClassSet fragments, BuildOptions options) { this.fragments = Preconditions.checkNotNull(fragments); - this.optionsDiff = Preconditions.checkNotNull(optionsDiff); - this.hashCode = Objects.hash(fragments, optionsDiff); + this.options = Preconditions.checkNotNull(options); + this.hashCode = Objects.hash(fragments, options); } @VisibleForTesting @@ -159,12 +156,8 @@ return fragments.fragmentClasses(); } - public BuildOptions.OptionsDiffForReconstruction getOptionsDiff() { - return optionsDiff; - } - - public String checksum() { - return optionsDiff.getChecksum(); + public BuildOptions getOptions() { + return options; } @Override @@ -181,7 +174,7 @@ return false; } Key otherConfig = (Key) o; - return optionsDiff.equals(otherConfig.optionsDiff) && fragments.equals(otherConfig.fragments); + return options.equals(otherConfig.options) && fragments.equals(otherConfig.fragments); } @Override @@ -194,14 +187,14 @@ // This format is depended on by integration tests. // TODO(blaze-configurability-team): This should at least include the length of fragments. // to at least remind devs that this Key has TWO key parts. - return "BuildConfigurationValue.Key[" + checksum() + "]"; + return "BuildConfigurationValue.Key[" + options.checksum() + "]"; } /** * Return a string representation that can be safely used for comparison purposes. * * <p>Unlike toString, which is short and good for printing in debug contexts, this is long - * because it includes sufficient information in optionsDiff and fragments. toString alone is + * because it includes sufficient information in options and fragments. toString alone is * insufficient because multiple Keys can have the same options checksum (and thus same * toString) but different fragments. * @@ -222,7 +215,7 @@ * BuildConfigurationValue. */ public String toComparableString() { - return "BuildConfigurationValue.Key[" + optionsDiff.getChecksum() + ", " + fragments + "]"; + return "BuildConfigurationValue.Key[" + options.checksum() + ", " + fragments + "]"; } } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index 5f03dc0..9510b0b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -273,7 +273,6 @@ computeUnloadedToolchainContexts( env, ruleClassProvider, - defaultBuildOptions, ctgValue, configuredTargetKey.getToolchainContextKey()); if (env.valuesMissing()) { @@ -457,7 +456,6 @@ static ToolchainCollection<UnloadedToolchainContext> computeUnloadedToolchainContexts( Environment env, RuleClassProvider ruleClassProvider, - BuildOptions defaultBuildOptions, TargetAndConfiguration targetAndConfig, @Nullable ToolchainContextKey parentToolchainContextKey) throws InterruptedException, ToolchainException { @@ -511,8 +509,7 @@ BuildConfigurationValue.Key toolchainConfig = BuildConfigurationValue.keyWithoutPlatformMapping( - configuration.getFragmentsMap().keySet(), - BuildOptions.diffForReconstruction(defaultBuildOptions, toolchainOptions)); + configuration.getFragmentsMap().keySet(), toolchainOptions); Map<String, ToolchainContextKey> toolchainContextKeys = new HashMap<>(); String targetUnloadedToolchainContext = "target-unloaded-toolchain-context";
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java index 7195a52..8a2caa3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
@@ -138,7 +138,8 @@ return "null"; } return String.format( - "%s (%s)", label, configurationKey == null ? "null" : configurationKey.checksum()); + "%s (%s)", + label, configurationKey == null ? "null" : configurationKey.getOptions().checksum()); } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java index d4d6de2..8efb2b8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java
@@ -198,8 +198,7 @@ private BuildConfigurationValue.Key computeMapping( BuildConfigurationValue.Key original, BuildOptions defaultBuildOptions) throws OptionsParsingException { - BuildOptions.OptionsDiffForReconstruction originalDiff = original.getOptionsDiff(); - BuildOptions originalOptions = defaultBuildOptions.applyDiff(originalDiff); + BuildOptions originalOptions = original.getOptions(); Preconditions.checkArgument( originalOptions.contains(PlatformOptions.class), @@ -242,8 +241,7 @@ } return BuildConfigurationValue.keyWithoutPlatformMapping( - original.getFragments(), - BuildOptions.diffForReconstruction(defaultBuildOptions, modifiedOptions)); + original.getFragments(), modifiedOptions); } private OptionsParsingResult parseWithCache( @@ -255,7 +253,7 @@ } } - private OptionsParsingResult parse(Iterable<String> args, BuildOptions defaultBuildOptions) + private static OptionsParsingResult parse(Iterable<String> args, BuildOptions defaultBuildOptions) throws OptionsParsingException { OptionsParser parser = OptionsParser.builder()
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java index 0d6f17c..9f621f9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java
@@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import com.google.common.base.Predicates; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -58,6 +57,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import javax.annotation.Nullable; @@ -80,7 +80,7 @@ throws InterruptedException, PrepareAnalysisPhaseFunctionException { PrepareAnalysisPhaseKey options = (PrepareAnalysisPhaseKey) key.argument(); - BuildOptions targetOptions = defaultBuildOptions.applyDiff(options.getOptionsDiff()); + BuildOptions targetOptions = options.getOptions(); BuildOptionsView hostTransitionOptionsView = new BuildOptionsView(targetOptions, HostTransition.INSTANCE.requiresOptionFragments()); BuildOptions hostOptions = @@ -98,24 +98,21 @@ return null; } - BuildConfigurationValue.Key hostConfigurationKey = null; + List<BuildOptions> topLevelBuildOptions = + getTopLevelBuildOptions(targetOptions, options.getMultiCpu()); + ImmutableList.Builder<BuildConfigurationValue.Key> targetConfigurationKeysBuilder = - ImmutableList.builder(); + ImmutableList.builderWithExpectedSize(topLevelBuildOptions.size()); + BuildConfigurationValue.Key hostConfigurationKey; try { hostConfigurationKey = BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, - defaultBuildOptions, - allFragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, hostOptions)); + platformMappingValue, defaultBuildOptions, allFragments, hostOptions); for (BuildOptions buildOptions : getTopLevelBuildOptions(targetOptions, options.getMultiCpu())) { targetConfigurationKeysBuilder.add( BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, - defaultBuildOptions, - allFragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, buildOptions))); + platformMappingValue, defaultBuildOptions, allFragments, buildOptions)); } } catch (OptionsParsingException e) { throw new PrepareAnalysisPhaseFunctionException(new InvalidConfigurationException(e)); @@ -135,10 +132,9 @@ // We only report invalid options for the target configurations, and abort if there's an error. ErrorSensingEventHandler<Void> nosyEventHandler = ErrorSensingEventHandler.withoutPropertyValueTracking(env.getListener()); - targetConfigurationKeys - .stream() - .map(k -> configs.get(k)) - .filter(Predicates.notNull()) + targetConfigurationKeys.stream() + .map(configs::get) + .filter(Objects::nonNull) .map(v -> ((BuildConfigurationValue) v).getConfiguration()) .forEach(config -> config.reportInvalidOptions(nosyEventHandler)); if (nosyEventHandler.hasErrors()) { @@ -247,13 +243,8 @@ LinkedHashSet<TargetAndConfiguration> result = new LinkedHashSet<>(); for (TargetAndConfiguration originalNode : nodes) { - if (successfullyEvaluatedTargets.containsKey(originalNode)) { - // The configuration was successfully trimmed. - result.add(successfullyEvaluatedTargets.get(originalNode)); - } else { - // Either the configuration couldn't be determined (e.g. loading phase error) or it's null. - result.add(originalNode); - } + // If the configuration couldn't be determined (e.g. loading phase error), use the original. + result.add(successfullyEvaluatedTargets.getOrDefault(originalNode, originalNode)); } return result; } @@ -280,7 +271,7 @@ } // Get the fragments needed for dynamic configuration nodes. - final List<SkyKey> transitiveFragmentSkyKeys = new ArrayList<>(); + List<SkyKey> transitiveFragmentSkyKeys = new ArrayList<>(); Map<Label, ImmutableSortedSet<Class<? extends Fragment>>> fragmentsMap = new HashMap<>(); Set<Label> labelsWithErrors = new HashSet<>(); for (DependencyKey key : keys) { @@ -298,21 +289,19 @@ } for (DependencyKey key : keys) { if (!depsToEvaluate.contains(key)) { - // No fragments to compute here. - } else { - TransitiveTargetKey targetKey = TransitiveTargetKey.of(key.getLabel()); - try { - TransitiveTargetValue ttv = - (TransitiveTargetValue) fragmentsResult.get(targetKey).get(); - fragmentsMap.put( - key.getLabel(), - ImmutableSortedSet.copyOf( - BuildConfiguration.lexicalFragmentSorter, - ttv.getTransitiveConfigFragments().toSet())); - } catch (NoSuchThingException e) { - // We silently skip any labels with errors - they'll be reported in the analysis phase. - labelsWithErrors.add(key.getLabel()); - } + continue; // No fragments to compute here. + } + TransitiveTargetKey targetKey = TransitiveTargetKey.of(key.getLabel()); + try { + TransitiveTargetValue ttv = (TransitiveTargetValue) fragmentsResult.get(targetKey).get(); + fragmentsMap.put( + key.getLabel(), + ImmutableSortedSet.copyOf( + BuildConfiguration.lexicalFragmentSorter, + ttv.getTransitiveConfigFragments().toSet())); + } catch (NoSuchThingException e) { + // We silently skip any labels with errors - they'll be reported in the analysis phase. + labelsWithErrors.add(key.getLabel()); } } @@ -348,10 +337,7 @@ for (BuildOptions toOption : toOptions) { configSkyKeys.add( BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, - defaultBuildOptions, - depFragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, toOption))); + platformMappingValue, defaultBuildOptions, depFragments, toOption)); } } } @@ -381,10 +367,7 @@ for (BuildOptions toOption : toOptions) { SkyKey configKey = BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, - defaultBuildOptions, - depFragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, toOption)); + platformMappingValue, defaultBuildOptions, depFragments, toOption); BuildConfigurationValue configValue = ((BuildConfigurationValue) configsResult.get(configKey)); // configValue will be null here if there was an exception thrown during configuration @@ -409,7 +392,7 @@ * {@link PrepareAnalysisPhaseFunction#compute}. */ private static final class PrepareAnalysisPhaseFunctionException extends SkyFunctionException { - public PrepareAnalysisPhaseFunctionException(InvalidConfigurationException e) { + PrepareAnalysisPhaseFunctionException(InvalidConfigurationException e) { super(e, Transience.PERSISTENT); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java index 374f263..2556a89 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java
@@ -106,7 +106,7 @@ skyframeExecutor.getConfigurations( eventHandler, topLevelCtKeys.stream() - .map(ctk -> ctk.getConfigurationKey()) + .map(ConfiguredTargetKey::getConfigurationKey) .filter(Predicates.notNull()) .collect(Collectors.toSet())); @@ -157,10 +157,10 @@ @ThreadSafe public static SkyKey key( FragmentClassSet fragments, - BuildOptions.OptionsDiffForReconstruction optionsDiff, + BuildOptions options, Set<String> multiCpu, Collection<Label> labels) { - return new PrepareAnalysisPhaseKey(fragments, optionsDiff, multiCpu, labels); + return new PrepareAnalysisPhaseKey(fragments, options, multiCpu, labels); } /** The configuration needed to prepare the analysis phase. */ @@ -169,17 +169,17 @@ @AutoCodec public static final class PrepareAnalysisPhaseKey implements SkyKey, Serializable { private final FragmentClassSet fragments; - private final BuildOptions.OptionsDiffForReconstruction optionsDiff; + private final BuildOptions options; private final ImmutableSortedSet<String> multiCpu; private final ImmutableSet<Label> labels; PrepareAnalysisPhaseKey( FragmentClassSet fragments, - BuildOptions.OptionsDiffForReconstruction optionsDiff, + BuildOptions options, Set<String> multiCpu, Collection<Label> labels) { this.fragments = Preconditions.checkNotNull(fragments); - this.optionsDiff = Preconditions.checkNotNull(optionsDiff); + this.options = Preconditions.checkNotNull(options); this.multiCpu = ImmutableSortedSet.copyOf(multiCpu); this.labels = ImmutableSet.copyOf(labels); } @@ -193,8 +193,8 @@ return fragments; } - public BuildOptions.OptionsDiffForReconstruction getOptionsDiff() { - return optionsDiff; + public BuildOptions getOptions() { + return options; } public ImmutableSortedSet<String> getMultiCpu() { @@ -209,7 +209,7 @@ public String toString() { return MoreObjects.toStringHelper(PrepareAnalysisPhaseKey.class) .add("fragments", fragments) - .add("optionsDiff", optionsDiff) + .add("optionsDiff", options) .add("multiCpu", multiCpu) .add("labels", labels) .toString(); @@ -217,11 +217,7 @@ @Override public int hashCode() { - return Objects.hash( - fragments, - optionsDiff, - multiCpu, - labels); + return Objects.hash(fragments, options, multiCpu, labels); } @Override @@ -234,7 +230,7 @@ } PrepareAnalysisPhaseKey other = (PrepareAnalysisPhaseKey) obj; return other.fragments.equals(this.fragments) - && other.optionsDiff.equals(this.optionsDiff) + && other.options.equals(this.options) && other.multiCpu.equals(multiCpu) && other.labels.equals(labels); }
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 713d543..7ecc59d 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
@@ -1077,8 +1077,7 @@ // case. So further optimization is necessary to make that viable (proto_library in particular // contributes to much of the difference). BuildConfiguration trimmedConfig = - topLevelHostConfiguration.clone( - fragmentClasses, ruleClassProvider, skyframeExecutor.getDefaultBuildOptions()); + topLevelHostConfiguration.clone(fragmentClasses, ruleClassProvider); hostConfigurationCache.put(config, trimmedConfig); return trimmedConfig; }
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 56715ae..ab9ce98 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
@@ -321,7 +321,7 @@ new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS); protected final AtomicReference<PathPackageLocator> pkgLocator = new AtomicReference<>(); protected final AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages = - new AtomicReference<>(ImmutableSet.<PackageIdentifier>of()); + new AtomicReference<>(ImmutableSet.of()); private final AtomicReference<EventBus> eventBus = new AtomicReference<>(); protected final AtomicReference<TimestampGranularityMonitor> tsgm = new AtomicReference<>(); protected final AtomicReference<Map<String, String>> clientEnv = new AtomicReference<>(); @@ -365,6 +365,7 @@ private final boolean shouldUnblockCpuWorkWhenFetchingDeps; + // TODO(b/185778053): This isn't needed anymore. Just use the fragments from ruleClassProvider. private final BuildOptions defaultBuildOptions; private PerBuildSyscallCache perBuildSyscallCache; @@ -592,7 +593,7 @@ new TopLevelActionLookupConflictFindingFunction()); map.put( SkyFunctions.BUILD_CONFIGURATION, - new BuildConfigurationFunction(directories, ruleClassProvider, defaultBuildOptions)); + new BuildConfigurationFunction(directories, ruleClassProvider)); map.put(SkyFunctions.WORKSPACE_NAME, new WorkspaceNameFunction()); map.put( WorkspaceFileValue.WORKSPACE_FILE, @@ -2175,10 +2176,7 @@ throws InvalidConfigurationException { try { return BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, - defaultBuildOptions, - depFragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, toOption)); + platformMappingValue, defaultBuildOptions, depFragments, toOption); } catch (OptionsParsingException e) { throw new InvalidConfigurationException(Code.INVALID_BUILD_OPTIONS, e); } @@ -2287,7 +2285,7 @@ getPlatformMappingValue(eventHandler, options), defaultBuildOptions, fragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, options)); + options); BuildConfigurationValue result = (BuildConfigurationValue) evaluate( @@ -3017,12 +3015,7 @@ .collect( ImmutableSortedSet.toImmutableSortedSet( BuildConfiguration.lexicalFragmentSorter))); - SkyKey key = - PrepareAnalysisPhaseValue.key( - allFragments, - BuildOptions.diffForReconstruction(defaultBuildOptions, buildOptions), - multiCpu, - labels); + SkyKey key = PrepareAnalysisPhaseValue.key(allFragments, buildOptions, multiCpu, labels); EvaluationResult<PrepareAnalysisPhaseValue> evalResult = evaluate( ImmutableList.of(key),
diff --git a/src/main/java/com/google/devtools/common/options/OptionsBase.java b/src/main/java/com/google/devtools/common/options/OptionsBase.java index cb5c8ba..3204e75 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsBase.java +++ b/src/main/java/com/google/devtools/common/options/OptionsBase.java
@@ -96,9 +96,9 @@ return result.append("}").toString(); } - public static String mapToCacheKey(Map<String, Object> optionsMap) { + public static String mapToCacheKey(Map<?, ?> optionsMap) { StringBuilder result = new StringBuilder(); - for (Map.Entry<String, Object> entry : optionsMap.entrySet()) { + for (Map.Entry<?, ?> entry : optionsMap.entrySet()) { result.append(entry.getKey()).append("="); Object value = entry.getValue();
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java index 0a4d020..5b83644 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java
@@ -211,8 +211,7 @@ ImmutableSortedSet.orderedBy(BuildConfiguration.lexicalFragmentSorter) .add(CppConfiguration.class) .build()), - analysisMock.createRuleClassProvider(), - skyframeExecutor.getDefaultBuildOptions()); + analysisMock.createRuleClassProvider()); BuildConfiguration hostConfig = createHost(); assertThat(config.equalsOrIsSupersetOf(trimmedConfig)).isTrue();
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java index ab45579..4a53f91 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
@@ -24,19 +24,18 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache; import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache; import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff; -import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiffForReconstruction; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.rules.android.AndroidConfiguration; import com.google.devtools.build.lib.rules.cpp.CppOptions; import com.google.devtools.build.lib.rules.java.JavaOptions; import com.google.devtools.build.lib.rules.proto.ProtoConfiguration; import com.google.devtools.build.lib.rules.python.PythonOptions; +import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; -import com.google.devtools.build.lib.skyframe.serialization.SerializationException; +import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester; import com.google.devtools.build.lib.skyframe.serialization.testutils.TestUtils; import com.google.devtools.common.options.OptionsParser; import com.google.protobuf.ByteString; -import java.io.IOException; import java.util.AbstractMap; import java.util.stream.Collectors; import org.junit.Test; @@ -52,6 +51,7 @@ */ @RunWith(JUnit4.class) public final class BuildOptionsTest { + private static final ImmutableList<Class<? extends FragmentOptions>> BUILD_CONFIG_OPTIONS = ImmutableList.of(CoreOptions.class); @@ -68,7 +68,8 @@ // The cache keys of the OptionSets must be equal even if these are // different objects, if they were created with the same options (no options in this case). assertThat(b.toString()).isEqualTo(a.toString()); - assertThat(b.computeCacheKey()).isEqualTo(a.computeCacheKey()); + assertThat(b.checksum()).isEqualTo(a.checksum()); + assertThat(a).isEqualTo(b); } @Test @@ -136,64 +137,10 @@ @Test public void optionsDiff_nullOptionsThrow() throws Exception { - BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); - BuildOptions two = null; - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> BuildOptions.diff(one, two)); - assertThat(e).hasMessageThat().contains("Cannot diff null BuildOptions"); - } - - @Test - public void optionsDiff_nullSecondValue() throws Exception { - BuildOptions one = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=gcc"); - BuildOptions two = BuildOptions.of(ImmutableList.of(CppOptions.class)); - OptionsDiffForReconstruction diffForReconstruction = - BuildOptions.diffForReconstruction(one, two); - OptionsDiff diff = BuildOptions.diff(one, two); - assertThat(diff.areSame()).isFalse(); - assertThat(diff.getSecond().values()).contains(null); - BuildOptions reconstructed = one.applyDiff(diffForReconstruction); - assertThat(reconstructed.get(CppOptions.class).cppCompiler).isNull(); - } - - @Test - public void optionsDiff_differentBaseThrowException() throws Exception { - BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); - BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8"); - BuildOptions three = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=gcc"); - OptionsDiffForReconstruction diffForReconstruction = - BuildOptions.diffForReconstruction(one, two); - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> three.applyDiff(diffForReconstruction)); - assertThat(e) - .hasMessageThat() - .contains("Cannot reconstruct BuildOptions with a different base"); - } - - @Test - public void optionsDiff_getEmptyAndApplyEmpty() throws Exception { - BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); - BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); - OptionsDiffForReconstruction diffForReconstruction = - BuildOptions.diffForReconstruction(one, two); - BuildOptions reconstructed = one.applyDiff(diffForReconstruction); - assertThat(one).isEqualTo(reconstructed); - } - - @Test - public void applyDiff_nativeOptions() throws Exception { - BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); - BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8"); - BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); - assertThat(reconstructedTwo).isEqualTo(two); - assertThat(reconstructedTwo).isNotSameInstanceAs(two); - BuildOptions reconstructedOne = one.applyDiff(BuildOptions.diffForReconstruction(one, one)); - assertThat(reconstructedOne).isSameInstanceAs(one); - BuildOptions otherFragment = BuildOptions.of(ImmutableList.of(CppOptions.class)); - assertThat(one.applyDiff(BuildOptions.diffForReconstruction(one, otherFragment))) - .isEqualTo(otherFragment); - assertThat(otherFragment.applyDiff(BuildOptions.diffForReconstruction(otherFragment, one))) - .isEqualTo(one); + BuildOptions options = + BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); + assertThrows(NullPointerException.class, () -> BuildOptions.diff(options, null)); + assertThrows(NullPointerException.class, () -> BuildOptions.diff(null, options)); } @Test @@ -241,81 +188,27 @@ } @Test - public void applyDiff_sameStarlarkOptions() { - Label flagName = Label.parseAbsoluteUnchecked("//foo/flag"); - String flagValue = "value"; - BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValue)); - BuildOptions two = BuildOptions.of(ImmutableMap.of(flagName, flagValue)); - - BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); - - assertThat(reconstructedTwo).isEqualTo(two); - assertThat(reconstructedTwo).isNotSameInstanceAs(two); - - BuildOptions reconstructedOne = one.applyDiff(BuildOptions.diffForReconstruction(one, one)); - - assertThat(reconstructedOne).isSameInstanceAs(one); - } - - @Test - public void applyDiff_differentStarlarkOptions() { - Label flagName = Label.parseAbsoluteUnchecked("//bar/flag"); - String flagValueOne = "valueOne"; - String flagValueTwo = "valueTwo"; - BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValueOne)); - BuildOptions two = BuildOptions.of(ImmutableMap.of(flagName, flagValueTwo)); - - BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); - - assertThat(reconstructedTwo).isEqualTo(two); - assertThat(reconstructedTwo).isNotSameInstanceAs(two); - } - - @Test - public void applyDiff_extraStarlarkOptions() { - Label flagNameOne = Label.parseAbsoluteUnchecked("//extra/flag/one"); - Label flagNameTwo = Label.parseAbsoluteUnchecked("//extra/flag/two"); - String flagValue = "foo"; - BuildOptions one = BuildOptions.of(ImmutableMap.of(flagNameOne, flagValue)); - BuildOptions two = BuildOptions.of(ImmutableMap.of(flagNameTwo, flagValue)); - - BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); - - assertThat(reconstructedTwo).isEqualTo(two); - assertThat(reconstructedTwo).isNotSameInstanceAs(two); - } - - @Test - public void applyDiff_returnsOriginalOptionsInstanceIfStillExists() throws Exception { - BuildOptions base = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=a"); - BuildOptions original = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=b"); - BuildOptions reconstructed = base.applyDiff(BuildOptions.diffForReconstruction(base, original)); - assertThat(reconstructed).isSameInstanceAs(original); - } - - @Test - public void diffForReconstruction_calledTwiceWithSameArgs_returnsSameInstance() throws Exception { - BuildOptions one = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=one"); - BuildOptions two = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=two"); - - OptionsDiffForReconstruction diff1 = BuildOptions.diffForReconstruction(one, two); - OptionsDiffForReconstruction diff2 = BuildOptions.diffForReconstruction(one, two); - - assertThat(diff1).isSameInstanceAs(diff2); - } - - @Test - public void diffForReconstruction_againstDifferentBase() throws Exception { - BuildOptions base1 = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=base1"); - BuildOptions base2 = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=base2"); - BuildOptions other = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=other"); - - OptionsDiffForReconstruction diff1 = BuildOptions.diffForReconstruction(base1, other); - OptionsDiffForReconstruction diff2 = BuildOptions.diffForReconstruction(base2, other); - - assertThat(diff1).isNotEqualTo(diff2); - assertThat(base1.applyDiff(diff1)).isEqualTo(other); - assertThat(base2.applyDiff(diff2)).isEqualTo(other); + public void serialization() throws Exception { + new SerializationTester( + BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"), + BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8"), + BuildOptions.of( + makeOptionsClassBuilder() + .add(AndroidConfiguration.Options.class) + .add(ProtoConfiguration.Options.class) + .build()), + BuildOptions.of( + makeOptionsClassBuilder().add(JavaOptions.class).add(PythonOptions.class).build(), + "--cpu=k8", + "--compilation_mode=opt", + "--compiler=gcc", + "--copt=-Dfoo", + "--javacopt=--javacoption", + "--experimental_fix_deps_tool=fake", + "--build_python_zip=no", + "--python_version=PY2")) + .addDependency(OptionsChecksumCache.class, new MapBackedChecksumCache()) + .runTests(); } private static ImmutableList.Builder<Class<? extends FragmentOptions>> makeOptionsClassBuilder() { @@ -324,43 +217,46 @@ .add(CppOptions.class); } - /** - * Tests that an {@link OptionsDiffForReconstruction} serializes stably. Unfortunately, still - * passes without fixes! (Perhaps more classes and diffs are needed?) - */ @Test - public void codecStability() throws Exception { - BuildOptions one = - BuildOptions.of( - makeOptionsClassBuilder() - .add(AndroidConfiguration.Options.class) - .add(ProtoConfiguration.Options.class) - .build()); - BuildOptions two = - BuildOptions.of( - makeOptionsClassBuilder().add(JavaOptions.class).add(PythonOptions.class).build(), - "--cpu=k8", - "--compilation_mode=opt", - "--compiler=gcc", - "--copt=-Dfoo", - "--javacopt=--javacoption", - "--experimental_fix_deps_tool=fake", - "--build_python_zip=no", - "--python_version=PY2"); - OptionsDiffForReconstruction diff1 = BuildOptions.diffForReconstruction(one, two); - OptionsDiffForReconstruction diff2 = BuildOptions.diffForReconstruction(one, two); - assertThat(diff2).isEqualTo(diff1); - assertThat(toBytes(diff2, new MapBackedChecksumCache())) - .isEqualTo(toBytes(diff1, new MapBackedChecksumCache())); + public void deserialize_unprimedCache_throws() throws Exception { + BuildOptions options = BuildOptions.of(BUILD_CONFIG_OPTIONS); + + SerializationContext serializationContext = + new SerializationContext( + ImmutableClassToInstanceMap.of( + OptionsChecksumCache.class, new MapBackedChecksumCache())); + ByteString bytes = TestUtils.toBytes(serializationContext, options); + assertThat(bytes).isNotNull(); + + // Different checksum cache than the one used for serialization, and it has not been primed. + DeserializationContext deserializationContext = + new DeserializationContext( + ImmutableClassToInstanceMap.of( + OptionsChecksumCache.class, new MapBackedChecksumCache())); + Exception e = + assertThrows( + RuntimeException.class, () -> TestUtils.fromBytes(deserializationContext, bytes)); + assertThat(e).hasMessageThat().contains(options.checksum()); } @Test - public void repeatedCodec() throws Exception { - BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); - BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8"); - OptionsDiffForReconstruction diff = BuildOptions.diffForReconstruction(one, two); - OptionsChecksumCache cache = new MapBackedChecksumCache(); - assertThat(toBytes(diff, cache)).isEqualTo(toBytes(diff, cache)); + public void deserialize_primedCache_returnsPrimedInstance() throws Exception { + BuildOptions options = BuildOptions.of(BUILD_CONFIG_OPTIONS); + + SerializationContext serializationContext = + new SerializationContext( + ImmutableClassToInstanceMap.of( + OptionsChecksumCache.class, new MapBackedChecksumCache())); + ByteString bytes = TestUtils.toBytes(serializationContext, options); + assertThat(bytes).isNotNull(); + + // Different checksum cache than the one used for serialization, but it has been primed. + OptionsChecksumCache checksumCache = new MapBackedChecksumCache(); + checksumCache.prime(options); + DeserializationContext deserializationContext = + new DeserializationContext( + ImmutableClassToInstanceMap.of(OptionsChecksumCache.class, checksumCache)); + assertThat(TestUtils.fromBytes(deserializationContext, bytes)).isSameInstanceAs(options); } @Test @@ -565,18 +461,4 @@ BuildOptions trimmed = original.trim(ImmutableSet.of(CppOptions.class, JavaOptions.class)); assertThat(trimmed).isSameInstanceAs(original); } - - private static OptionsDiffForReconstruction uncachedDiffForReconstruction( - BuildOptions first, BuildOptions second) { - OptionsDiffForReconstruction diff = BuildOptions.diffForReconstruction(first, second); - diff.clearCachedReconstructedForTesting(); - return diff; - } - - private static ByteString toBytes(OptionsDiffForReconstruction diff, OptionsChecksumCache cache) - throws IOException, SerializationException { - return TestUtils.toBytes( - new SerializationContext(ImmutableClassToInstanceMap.of(OptionsChecksumCache.class, cache)), - diff); - } }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java index 1bc1d90..b324080 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.util.ConfigurationTestCase; +import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.TriState; import org.junit.Test; @@ -30,6 +31,7 @@ @RunWith(JUnit4.class) public class PythonConfigurationTest extends ConfigurationTestCase { + // Do not mutate the returned PythonOptions - it will poison skyframe caches. private PythonOptions parsePythonOptions(String... cmdline) throws Exception { BuildConfiguration config = create(cmdline); return config.getOptions().get(PythonOptions.class); @@ -82,7 +84,7 @@ @Test public void setPythonVersion() throws Exception { - PythonOptions opts = parsePythonOptions("--python_version=PY2"); + PythonOptions opts = Options.parse(PythonOptions.class, "--python_version=PY2").getOptions(); opts.setPythonVersion(PythonVersion.PY3); assertThat(opts.pythonVersion).isEqualTo(PythonVersion.PY3); }
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java index 4c6cf98..02d4656 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
@@ -944,15 +944,14 @@ new BlazeDirectories( new ServerDirectories(outputBase, outputBase, outputBase), rootDirectory, - /* defaultSystemJavabase= */ null, + /*defaultSystemJavabase=*/ null, "productName"), - /* fragmentsMap= */ ImmutableMap.of(), + /*fragmentsMap=*/ ImmutableMap.of(), defaultBuildOptions, - BuildOptions.diffForReconstruction(defaultBuildOptions, defaultBuildOptions), - /* reservedActionMnemonics= */ ImmutableSet.of(), + /*reservedActionMnemonics=*/ ImmutableSet.of(), ActionEnvironment.EMPTY, "workspace", - /* siblingRepositoryLayout= */ false); + /*siblingRepositoryLayout=*/ false); BuildEvent firstWithConfiguration = new GenericConfigurationEvent(testId("first"), configuration.toBuildEvent()); BuildEvent secondWithConfiguration =
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java index 9a153e2..321bed1 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java
@@ -44,7 +44,7 @@ * <p>Note that all parsing tests are located in {@link PlatformMappingFunctionParserTest}. */ @RunWith(JUnit4.class) -public class PlatformMappingFunctionTest extends BuildViewTestCase { +public final class PlatformMappingFunctionTest extends BuildViewTestCase { // We don't actually care about the contents of this set other than that it is passed intact // through the mapping logic. The platform fragment in it is purely an example, it could be any @@ -59,14 +59,11 @@ private static final BuildOptions DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS = getDefaultBuildConfigPlatformOptions(); - private static final BuildOptions.OptionsDiffForReconstruction EMPTY_DIFF = - BuildOptions.diffForReconstruction( - DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); private static final Label DEFAULT_TARGET_PLATFORM = Label.parseAbsoluteUnchecked("@local_config_platform//:host"); @Test - public void testMappingFileDoesNotExist() throws Exception { + public void testMappingFileDoesNotExist() { MissingInputFileException exception = assertThrows( MissingInputFileException.class, @@ -82,12 +79,13 @@ executeFunction(PlatformMappingValue.Key.create(null)); BuildConfigurationValue.Key key = - BuildConfigurationValue.keyWithoutPlatformMapping(PLATFORM_FRAGMENT_CLASS, EMPTY_DIFF); + BuildConfigurationValue.keyWithoutPlatformMapping( + PLATFORM_FRAGMENT_CLASS, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); BuildConfigurationValue.Key mapped = platformMappingValue.map(key, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms) + assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); } @@ -120,7 +118,7 @@ platformMappingValue.map( keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -144,7 +142,7 @@ platformMappingValue.map( keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -166,7 +164,7 @@ platformMappingValue.map( keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -189,7 +187,7 @@ platformMappingValue.map( keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -217,7 +215,7 @@ platformMappingValue.map( keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } // Internal flags, such as "output directory name", cannot be set from the command-line, but @@ -240,7 +238,7 @@ platformMappingValue.map( keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(CoreOptions.class).outputDirectoryName) + assertThat(mapped.getOptions().get(CoreOptions.class).outputDirectoryName) .isEqualTo("updated_output_dir"); } @@ -258,10 +256,6 @@ return result.get(key); } - private static BuildOptions toMappedOptions(BuildConfigurationValue.Key mapped) { - return DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.applyDiff(mapped.getOptionsDiff()); - } - private static BuildOptions getDefaultBuildConfigPlatformOptions() { try { return BuildOptions.of(BUILD_CONFIG_PLATFORM_OPTIONS); @@ -271,9 +265,7 @@ } private static BuildConfigurationValue.Key keyForOptions(BuildOptions modifiedOptions) { - BuildOptions.OptionsDiffForReconstruction diff = - BuildOptions.diffForReconstruction(DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS, modifiedOptions); - - return BuildConfigurationValue.keyWithoutPlatformMapping(PLATFORM_FRAGMENT_CLASS, diff); + return BuildConfigurationValue.keyWithoutPlatformMapping( + PLATFORM_FRAGMENT_CLASS, modifiedOptions); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java index 4be3455..ea8ef5b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java
@@ -36,7 +36,7 @@ /** Unit tests for {@link PlatformMappingValue}. */ @RunWith(JUnit4.class) -public class PlatformMappingValueTest { +public final class PlatformMappingValueTest { // We don't actually care about the contents of this set other than that it is passed intact // through the mapping logic. The platform fragment in it is purely an example, it could be any @@ -52,9 +52,6 @@ private static final BuildOptions DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS = getDefaultBuildConfigPlatformOptions(); - private static final BuildOptions.OptionsDiffForReconstruction EMPTY_DIFF = - BuildOptions.diffForReconstruction( - DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); private static final Label DEFAULT_TARGET_PLATFORM = Label.parseAbsoluteUnchecked("@local_config_platform//:host"); @@ -64,12 +61,13 @@ new PlatformMappingValue(ImmutableMap.of(), ImmutableMap.of()); BuildConfigurationValue.Key key = - BuildConfigurationValue.keyWithoutPlatformMapping(PLATFORM_FRAGMENT_CLASS, EMPTY_DIFF); + BuildConfigurationValue.keyWithoutPlatformMapping( + PLATFORM_FRAGMENT_CLASS, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); BuildConfigurationValue.Key mapped = mappingValue.map(key, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms) + assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); } @@ -89,7 +87,7 @@ assertThat(mapped.getFragments()).isEqualTo(PLATFORM_FRAGMENT_CLASS); - assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -109,8 +107,7 @@ assertThat(mapped.getFragments()).isEqualTo(PLATFORM_FRAGMENT_CLASS); - assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms) - .containsExactly(PLATFORM1); + assertThat(mapped.getOptions().get(PlatformOptions.class).platforms).containsExactly(PLATFORM1); } @Test @@ -129,8 +126,7 @@ BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms) - .containsExactly(PLATFORM2); + assertThat(mapped.getOptions().get(PlatformOptions.class).platforms).containsExactly(PLATFORM2); } @Test @@ -147,7 +143,7 @@ BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms) + assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); } @@ -221,10 +217,6 @@ assertThat(key.wasExplicitlySetByUser()).isTrue(); } - private static BuildOptions toMappedOptions(BuildConfigurationValue.Key mapped) { - return DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.applyDiff(mapped.getOptionsDiff()); - } - private static BuildOptions getDefaultBuildConfigPlatformOptions() { try { return BuildOptions.of(BUILD_CONFIG_PLATFORM_OPTIONS); @@ -234,9 +226,7 @@ } private static BuildConfigurationValue.Key keyForOptions(BuildOptions modifiedOptions) { - BuildOptions.OptionsDiffForReconstruction diff = - BuildOptions.diffForReconstruction(DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS, modifiedOptions); - - return BuildConfigurationValue.keyWithoutPlatformMapping(PLATFORM_FRAGMENT_CLASS, diff); + return BuildConfigurationValue.keyWithoutPlatformMapping( + PLATFORM_FRAGMENT_CLASS, modifiedOptions); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java index 3e3066d..4a8bf8d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java
@@ -22,7 +22,6 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.TargetAndConfiguration; import com.google.devtools.build.lib.analysis.ToolchainCollection; -import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.AnalysisTestCase; import com.google.devtools.build.lib.cmdline.Label; @@ -36,7 +35,6 @@ import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; -import java.util.function.Supplier; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -104,13 +102,9 @@ "CONFIGURED_TARGET_FUNCTION_COMPUTE_UNLOADED_TOOLCHAIN_CONTEXTS"); private final LateBoundStateProvider stateProvider; - private final Supplier<BuildOptions> buildOptionsSupplier; - ComputeUnloadedToolchainContextsFunction( - LateBoundStateProvider lateBoundStateProvider, - Supplier<BuildOptions> buildOptionsSupplier) { + ComputeUnloadedToolchainContextsFunction(LateBoundStateProvider lateBoundStateProvider) { this.stateProvider = lateBoundStateProvider; - this.buildOptionsSupplier = buildOptionsSupplier; } @Override @@ -122,7 +116,6 @@ ConfiguredTargetFunction.computeUnloadedToolchainContexts( env, stateProvider.lateBoundRuleClassProvider(), - buildOptionsSupplier.get(), key.targetAndConfiguration(), key.configuredTargetKey().getToolchainContextKey()); return env.valuesMissing() ? null : Value.create(toolchainCollection); @@ -162,15 +155,10 @@ */ private static final class AnalysisMockWithComputeDepsFunction extends AnalysisMock.Delegate { private final LateBoundStateProvider stateProvider; - private final Supplier<BuildOptions> defaultBuildOptions; - AnalysisMockWithComputeDepsFunction( - AnalysisMock parent, - LateBoundStateProvider stateProvider, - Supplier<BuildOptions> defaultBuildOptions) { + AnalysisMockWithComputeDepsFunction(AnalysisMock parent, LateBoundStateProvider stateProvider) { super(parent); this.stateProvider = stateProvider; - this.defaultBuildOptions = defaultBuildOptions; } @Override @@ -180,7 +168,7 @@ .putAll(super.getSkyFunctions(directories)) .put( ComputeUnloadedToolchainContextsFunction.SKYFUNCTION_NAME, - new ComputeUnloadedToolchainContextsFunction(stateProvider, defaultBuildOptions)) + new ComputeUnloadedToolchainContextsFunction(stateProvider)) .build(); } } @@ -188,9 +176,7 @@ @Override protected AnalysisMock getAnalysisMock() { return new AnalysisMockWithComputeDepsFunction( - super.getAnalysisMock(), - new LateBoundStateProvider(), - () -> skyframeExecutor.getDefaultBuildOptions()); + super.getAnalysisMock(), new LateBoundStateProvider()); } public ToolchainCollection<UnloadedToolchainContext> getToolchainCollection( @@ -204,13 +190,11 @@ // Analysis phase ended after the update() call in getToolchainCollection. We must re-enable // analysis so we can call ConfiguredTargetFunction again without raising an error. skyframeExecutor.getSkyframeBuildView().enableAnalysis(true); - Object evalResult = + EvaluationResult<Value> evalResult = SkyframeExecutorTestUtils.evaluate(skyframeExecutor, key, /*keepGoing=*/ false, reporter); // Test call has finished, to reset the state. skyframeExecutor.getSkyframeBuildView().enableAnalysis(false); - @SuppressWarnings("unchecked") - SkyValue value = ((EvaluationResult<Value>) evalResult).get(key); - return ((Value) value).getToolchainCollection(); + return evalResult.get(key).getToolchainCollection(); } public ToolchainCollection<UnloadedToolchainContext> getToolchainCollection(String targetLabel)