Automated rollback of commit db01c6f.

*** Reason for rollback ***

Rolling forward with fixes.

*** Original change description ***

PiperOrigin-RevId: 206339696

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 @@ public List<String> getPrettyPrintList() {
    * 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 String toString() {
     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);
+    }
   }
 }

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 BuildConfiguration getConfiguration() {
   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 @@ public static Key key(BuildConfiguration buildConfiguration) {
   }
 
   /** {@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 int hashCode() {
     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() {}
   }
 }

src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java

@@ -462,9 +462,7 @@ public void testCodec() throws Exception {
     // 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 @@ public void testKeyCodec() throws Exception {
                 .stream()
                 .map(BuildConfigurationValue::key)
                 .collect(ImmutableList.toImmutableList()))
-        .addDependency(
-            BuildConfigurationValue.KeyCodecCache.class,
-            new BuildConfigurationValue.KeyCodecCache())
+        .addDependency(BuildOptions.OptionsDiffCache.class, new BuildOptions.DiffToByteCache())
         .runTests();
   }