Automated rollback of commit db01c6f926bcb4774d901797c59f51dd54c05624.
*** Reason for rollback ***
Rolling forward with fixes.
*** Original change description ***
PiperOrigin-RevId: 206339696
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 81c3d9b..5fc56d8 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
@@ -27,6 +27,10 @@
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+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.SerializationException;
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;
@@ -36,6 +40,10 @@
import com.google.devtools.common.options.OptionsClassProvider;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
+import com.google.protobuf.ByteString;
+import com.google.protobuf.CodedInputStream;
+import com.google.protobuf.CodedOutputStream;
+import java.io.IOException;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Arrays;
@@ -50,6 +58,8 @@
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.logging.Logger;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
@@ -60,6 +70,7 @@
public final class BuildOptions implements Cloneable, Serializable {
private static final Comparator<Class<? extends FragmentOptions>>
lexicalFragmentOptionsComparator = Comparator.comparing(Class::getName);
+ private static final Logger logger = Logger.getLogger(BuildOptions.class.getName());
/**
* Creates a BuildOptions object with all options set to their default values, processed by the
@@ -524,7 +535,6 @@
* 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.
*/
- @AutoCodec
public static class OptionsDiffForReconstruction {
private final Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions;
private final ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses;
@@ -608,5 +618,186 @@
public int hashCode() {
return 31 * Arrays.hashCode(baseFingerprint) + checksum.hashCode();
}
+
+ private static class Codec implements ObjectCodec<OptionsDiffForReconstruction> {
+
+ @Override
+ public Class<OptionsDiffForReconstruction> getEncodedClass() {
+ return OptionsDiffForReconstruction.class;
+ }
+
+ @Override
+ public void serialize(
+ SerializationContext context,
+ OptionsDiffForReconstruction diff,
+ CodedOutputStream codedOut)
+ throws SerializationException, IOException {
+ OptionsDiffCache cache = context.getDependency(OptionsDiffCache.class);
+ ByteString bytes = cache.getBytesFromOptionsDiff(diff);
+ if (bytes == null) {
+ context = context.getNewNonMemoizingContext();
+ ByteString.Output byteStringOut = ByteString.newOutput();
+ CodedOutputStream bytesOut = CodedOutputStream.newInstance(byteStringOut);
+ context.serialize(diff.differingOptions, bytesOut);
+ context.serialize(diff.extraFirstFragmentClasses, bytesOut);
+ context.serialize(diff.extraSecondFragments, bytesOut);
+ bytesOut.writeByteArrayNoTag(diff.baseFingerprint);
+ context.serialize(diff.checksum, bytesOut);
+ bytesOut.flush();
+ byteStringOut.flush();
+ int optionsDiffSize = byteStringOut.size();
+ bytes = byteStringOut.toByteString();
+ cache.putBytesFromOptionsDiff(diff, bytes);
+ logger.info(
+ "Serialized OptionsDiffForReconstruction "
+ + diff.toString()
+ + ". Diff took "
+ + optionsDiffSize
+ + " bytes.");
+ }
+ codedOut.writeBytesNoTag(bytes);
+ }
+
+ @Override
+ public OptionsDiffForReconstruction deserialize(
+ DeserializationContext context, CodedInputStream codedIn)
+ throws SerializationException, IOException {
+ OptionsDiffCache cache = context.getDependency(OptionsDiffCache.class);
+ ByteString bytes = codedIn.readBytes();
+ OptionsDiffForReconstruction diff = cache.getOptionsDiffFromBytes(bytes);
+ if (diff == null) {
+ CodedInputStream codedInput = bytes.newCodedInput();
+ context = context.getNewNonMemoizingContext();
+ Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions =
+ context.deserialize(codedInput);
+ ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses =
+ context.deserialize(codedInput);
+ ImmutableList<FragmentOptions> extraSecondFragments = context.deserialize(codedInput);
+ byte[] baseFingerprint = codedInput.readByteArray();
+ String checksum = context.deserialize(codedInput);
+ diff =
+ new OptionsDiffForReconstruction(
+ differingOptions,
+ extraFirstFragmentClasses,
+ extraSecondFragments,
+ baseFingerprint,
+ checksum);
+ cache.putBytesFromOptionsDiff(diff, bytes);
+ }
+ return diff;
+ }
+ }
+ }
+
+ /**
+ * Injected cache for {@code Codec}, so that we don't have to repeatedly serialize the same
+ * object. We still incur the over-the-wire cost of the bytes, but we don't use CPU to repeatedly
+ * compute it.
+ *
+ * <p>We provide the cache as an injected dependency so that different serializers' caches are
+ * isolated.
+ *
+ * <p>Used when configured targets are serialized: the more compact {@link
+ * FingerprintingKDiffToByteStringCache} cache below cannot be easily used because a configured
+ * target may have an edge to a configured target in a different configuration, and with only the
+ * checksum there would be no way to compute that configuration (although it is very likely in the
+ * graph already).
+ */
+ public interface OptionsDiffCache {
+ ByteString getBytesFromOptionsDiff(OptionsDiffForReconstruction diff);
+
+ void putBytesFromOptionsDiff(OptionsDiffForReconstruction diff, ByteString bytes);
+
+ OptionsDiffForReconstruction getOptionsDiffFromBytes(ByteString bytes);
+ }
+
+ /**
+ * Implementation of the {@link OptionsDiffCache} that acts as a {@code BiMap} utilizing two
+ * {@code ConcurrentHashMaps}.
+ */
+ public static class DiffToByteCache implements OptionsDiffCache {
+ // We expect there to be very few elements so keeping the reverse map as well as the forward
+ // map should be very cheap.
+ private static final ConcurrentHashMap<OptionsDiffForReconstruction, ByteString>
+ diffToByteStringMap = new ConcurrentHashMap<>();
+ private static final ConcurrentHashMap<ByteString, OptionsDiffForReconstruction>
+ byteStringToDiffMap = new ConcurrentHashMap<>();
+
+ @VisibleForTesting
+ public DiffToByteCache() {}
+
+ @Override
+ public ByteString getBytesFromOptionsDiff(OptionsDiffForReconstruction diff) {
+ return diffToByteStringMap.get(diff);
+ }
+
+ @Override
+ public void putBytesFromOptionsDiff(OptionsDiffForReconstruction diff, ByteString bytes) {
+ // We need to insert data into map that will be used for deserialization first in case there
+ // is a race between two threads. If we populated the diffToByteStringMap first, another
+ // thread could get the result above and race ahead, but then get a cache miss on
+ // deserialization.
+ byteStringToDiffMap.put(bytes, diff);
+ diffToByteStringMap.put(diff, bytes);
+ }
+
+ @Override
+ public OptionsDiffForReconstruction getOptionsDiffFromBytes(ByteString bytes) {
+ return byteStringToDiffMap.get(bytes);
+ }
+ }
+
+ /**
+ * {@link BuildOptions.OptionsDiffForReconstruction} serialization cache that relies on only
+ * serializing the checksum string instead of the full OptionsDiffForReconstruction.
+ *
+ * <p>This requires that every {@link BuildOptions.OptionsDiffForReconstruction} instance
+ * encountered is serialized <i>before</i> it is ever deserialized. When not serializing
+ * configured targets, this holds because every configuration present in the build is looked up in
+ * the graph using a {@code BuildConfigurationValue.Key}, which contains its {@link
+ * BuildOptions.OptionsDiffForReconstruction}. This requires that {@code BuildConfigurationValue}
+ * instances must always be serialized.
+ */
+ public static class FingerprintingKDiffToByteStringCache
+ implements BuildOptions.OptionsDiffCache {
+ private static final ConcurrentHashMap<OptionsDiffForReconstruction, ByteString>
+ diffToByteStringCache = new ConcurrentHashMap<>();
+ private static final ConcurrentHashMap<ByteString, OptionsDiffForReconstruction>
+ byteStringToDiffMap = new ConcurrentHashMap<>();
+
+ @Override
+ public ByteString getBytesFromOptionsDiff(OptionsDiffForReconstruction diff) {
+ ByteString checksumString = diffToByteStringCache.get(diff);
+ if (checksumString != null) {
+ // Fast path to avoid ByteString creation churn and unnecessary map insertions.
+ return checksumString;
+ }
+ checksumString = ByteString.copyFromUtf8(diff.getChecksum());
+ // We need to insert data into map that will be used for deserialization first in case there
+ // is a race between two threads. If we populated the diffToByteStringCache first, another
+ // thread could get checksumString above during serialization and race ahead, but then get a
+ // cache miss on deserialization.
+ byteStringToDiffMap.put(checksumString, diff);
+ diffToByteStringCache.put(diff, checksumString);
+ return checksumString;
+ }
+
+ @Override
+ public void putBytesFromOptionsDiff(OptionsDiffForReconstruction diff, ByteString bytes) {
+ throw new UnsupportedOperationException(
+ "diff "
+ + diff
+ + " should have not been serialized: "
+ + diffToByteStringCache
+ + ", "
+ + byteStringToDiffMap);
+ }
+
+ @Override
+ public OptionsDiffForReconstruction getOptionsDiffFromBytes(ByteString bytes) {
+ return Preconditions.checkNotNull(
+ byteStringToDiffMap.get(bytes),
+ "Missing bytes " + bytes + ": " + diffToByteStringCache + ", " + byteStringToDiffMap);
+ }
}
}
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 1e9bc52..90479a9 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
@@ -21,23 +21,14 @@
import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-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.SerializationException;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-import com.google.protobuf.ByteString;
-import com.google.protobuf.CodedInputStream;
-import com.google.protobuf.CodedOutputStream;
-import java.io.IOException;
import java.io.Serializable;
import java.util.Objects;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
import java.util.logging.Logger;
/** A Skyframe value representing a {@link BuildConfiguration}. */
@@ -68,7 +59,7 @@
public static Key key(
Set<Class<? extends BuildConfiguration.Fragment>> fragments,
BuildOptions.OptionsDiffForReconstruction optionsDiff) {
- return key(
+ return Key.create(
FragmentClassSet.of(
ImmutableSortedSet.copyOf(BuildConfiguration.lexicalFragmentSorter, fragments)),
optionsDiff);
@@ -84,15 +75,18 @@
}
/** {@link SkyKey} for {@link BuildConfigurationValue}. */
+ @AutoCodec
public static final class Key implements SkyKey, Serializable {
private static final Interner<Key> keyInterner = BlazeInterners.newWeakInterner();
private final FragmentClassSet fragments;
- private final BuildOptions.OptionsDiffForReconstruction optionsDiff;
+ final BuildOptions.OptionsDiffForReconstruction optionsDiff;
// If hashCode really is -1, we'll recompute it from scratch each time. Oh well.
private volatile int hashCode = -1;
- private static Key create(
+ @AutoCodec.Instantiator
+ @VisibleForSerialization
+ static Key create(
FragmentClassSet fragments, BuildOptions.OptionsDiffForReconstruction optionsDiff) {
return keyInterner.intern(new Key(fragments, optionsDiff));
}
@@ -141,71 +135,5 @@
public String toString() {
return "BuildConfigurationValue.Key[" + optionsDiff.getChecksum() + "]";
}
-
- private static class Codec implements ObjectCodec<Key> {
- @Override
- public Class<Key> getEncodedClass() {
- return Key.class;
- }
-
- @Override
- public void serialize(SerializationContext context, Key obj, CodedOutputStream codedOut)
- throws SerializationException, IOException {
- @SuppressWarnings("unchecked")
- ConcurrentMap<BuildConfigurationValue.Key, ByteString> cache =
- context.getDependency(KeyCodecCache.class).map;
- ByteString bytes = cache.get(obj);
- if (bytes == null) {
- context = context.getNewNonMemoizingContext();
- ByteString.Output byteStringOut = ByteString.newOutput();
- CodedOutputStream bytesOut = CodedOutputStream.newInstance(byteStringOut);
- context.serialize(obj.optionsDiff, bytesOut);
- bytesOut.flush();
- byteStringOut.flush();
- int optionsDiffSerializedSize = byteStringOut.size();
- context.serialize(obj.fragments, bytesOut);
- bytesOut.flush();
- byteStringOut.flush();
- bytes = byteStringOut.toByteString();
- cache.put(obj, bytes);
- logger.info(
- "Serialized "
- + obj.optionsDiff
- + " and "
- + obj.fragments
- + " to "
- + bytes.size()
- + " bytes (optionsDiff took "
- + optionsDiffSerializedSize
- + " bytes)");
- }
- codedOut.writeBytesNoTag(bytes);
- }
-
- @Override
- public Key deserialize(DeserializationContext context, CodedInputStream codedIn)
- throws SerializationException, IOException {
- codedIn = codedIn.readBytes().newCodedInput();
- context = context.getNewNonMemoizingContext();
- BuildOptions.OptionsDiffForReconstruction optionsDiff = context.deserialize(codedIn);
- FragmentClassSet fragmentClassSet = context.deserialize(codedIn);
- return key(fragmentClassSet, optionsDiff);
- }
- }
- }
-
- /**
- * Injected cache for {@code Codec}, so that we don't have to repeatedly serialize the same
- * object. We still incur the over-the-wire cost of the bytes, but we don't use CPU to repeatedly
- * compute it.
- *
- * <p>We provide the cache as an injected dependency so that different serializers' caches are
- * isolated.
- */
- public static class KeyCodecCache {
- private final ConcurrentMap<BuildConfigurationValue.Key, ByteString> map =
- new ConcurrentHashMap<>();
-
- public KeyCodecCache() {}
}
}
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 baa2940..3b280c5 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
@@ -462,9 +462,7 @@
// Unnecessary ImmutableList.copyOf apparently necessary to choose non-varargs constructor.
new SerializationTester(ImmutableList.copyOf(getTestConfigurations()))
.addDependency(FileSystem.class, getScratch().getFileSystem())
- .addDependency(
- BuildConfigurationValue.KeyCodecCache.class,
- new BuildConfigurationValue.KeyCodecCache())
+ .addDependency(BuildOptions.OptionsDiffCache.class, new BuildOptions.DiffToByteCache())
.setVerificationFunction(BuildConfigurationTest::verifyDeserialized)
.runTests();
}
@@ -476,9 +474,7 @@
.stream()
.map(BuildConfigurationValue::key)
.collect(ImmutableList.toImmutableList()))
- .addDependency(
- BuildConfigurationValue.KeyCodecCache.class,
- new BuildConfigurationValue.KeyCodecCache())
+ .addDependency(BuildOptions.OptionsDiffCache.class, new BuildOptions.DiffToByteCache())
.runTests();
}
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 3f0f93a..099ded0 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
@@ -210,7 +210,26 @@
OptionsDiffForReconstruction diff1 = BuildOptions.diffForReconstruction(one, two);
OptionsDiffForReconstruction diff2 = BuildOptions.diffForReconstruction(one, two);
assertThat(diff2).isEqualTo(diff1);
- assertThat(TestUtils.toBytes(diff2, ImmutableMap.of()))
- .isEqualTo(TestUtils.toBytes(diff1, ImmutableMap.of()));
+ assertThat(
+ TestUtils.toBytes(
+ diff2,
+ ImmutableMap.of(
+ BuildOptions.OptionsDiffCache.class, new BuildOptions.DiffToByteCache())))
+ .isEqualTo(
+ TestUtils.toBytes(
+ diff1,
+ ImmutableMap.of(
+ BuildOptions.OptionsDiffCache.class, new BuildOptions.DiffToByteCache())));
+ }
+
+ @Test
+ public void repeatedCodec() throws Exception {
+ BuildOptions one = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=opt", "cpu=k8");
+ BuildOptions two = BuildOptions.of(TEST_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
+ OptionsDiffForReconstruction diff = BuildOptions.diffForReconstruction(one, two);
+ BuildOptions.OptionsDiffCache cache = new BuildOptions.FingerprintingKDiffToByteStringCache();
+ assertThat(TestUtils.toBytes(diff, ImmutableMap.of(BuildOptions.OptionsDiffCache.class, cache)))
+ .isEqualTo(
+ TestUtils.toBytes(diff, ImmutableMap.of(BuildOptions.OptionsDiffCache.class, cache)));
}
}
