Don't do serialization of empty/singleton nested sets into a child CodedOutputStream. It creates immense amounts of garbage and we don't ever use the result: it's only used for Object[] children anyway.

We can consider removing the child CodedOutputStream entirely and relying on normal serialization memoization, but for now, let's just do the simple thing.

Also fix a weird code-only bug that had been there since NestedSetCodec was written (I think): NestedSet.EMPTY_CHILDREN is an Object[], and therefore we never took the fast path of just writing 0 and moving on. While the code as written was misleading, the bits written to the output stream were the same, until this change, when there was a divergence.

PiperOrigin-RevId: 191520712

1 files changed, 30 insertions, 29 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodec.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodec.java
index 14369814af..c564f32f19 100644
--- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodec.java
+++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodec.java
@@ -102,25 +102,26 @@ public class NestedSetCodec<T> implements ObjectCodec<NestedSet<T>> {
       CodedOutputStream codedOut,
       Map<Object, byte[]> childToDigest)
       throws IOException, SerializationException {
-    // Serialize nested set into an inner byte array so we can take its digest
-    ByteArrayOutputStream childOutputStream = new ByteArrayOutputStream();
-    HashingOutputStream hashingOutputStream =
-        new HashingOutputStream(Hashing.md5(), childOutputStream);
-    CodedOutputStream childCodedOut = CodedOutputStream.newInstance(hashingOutputStream);
-    if (children instanceof Object[]) {
+    if (children == NestedSet.EMPTY_CHILDREN) {
+      // Empty set
+      codedOut.writeInt32NoTag(0);
+    } else if (children instanceof Object[]) {
+      // Serialize nested set into an inner byte array so we can take its digest.
+      ByteArrayOutputStream childOutputStream = new ByteArrayOutputStream();
+      HashingOutputStream hashingOutputStream =
+          new HashingOutputStream(Hashing.md5(), childOutputStream);
+      CodedOutputStream childCodedOut = CodedOutputStream.newInstance(hashingOutputStream);
       serializeMultiItemChildArray(context, (Object[]) children, childToDigest, childCodedOut);
-    } else if (children != NestedSet.EMPTY_CHILDREN) {
-      serializeSingleItemChildArray(context, children, childCodedOut);
+      childCodedOut.flush();
+      byte[] digest = hashingOutputStream.hash().asBytes();
+      codedOut.writeInt32NoTag(((Object[]) children).length);
+      codedOut.writeByteArrayNoTag(digest);
+      byte[] childBytes = childOutputStream.toByteArray();
+      codedOut.writeByteArrayNoTag(childBytes);
+      childToDigest.put(children, digest);
     } else {
-      // Empty set
-      childCodedOut.writeInt32NoTag(0);
+      serializeSingleItemChildArray(context, children, codedOut);
     }
-    childCodedOut.flush();
-    byte[] digest = hashingOutputStream.hash().asBytes();
-    codedOut.writeByteArrayNoTag(digest);
-    byte[] childBytes = childOutputStream.toByteArray();
-    codedOut.writeByteArrayNoTag(childBytes);
-    childToDigest.put(children, digest);
   }
 
   private void serializeMultiItemChildArray(
@@ -129,7 +130,6 @@ public class NestedSetCodec<T> implements ObjectCodec<NestedSet<T>> {
       Map<Object, byte[]> childToDigest,
       CodedOutputStream childCodedOut)
       throws IOException, SerializationException {
-    childCodedOut.writeInt32NoTag(children.length);
     for (Object child : children) {
       if (child instanceof Object[]) {
         byte[] digest =
@@ -146,11 +146,11 @@ public class NestedSetCodec<T> implements ObjectCodec<NestedSet<T>> {
   }
 
   private void serializeSingleItemChildArray(
-      SerializationContext context, Object children, CodedOutputStream childCodedOut)
+      SerializationContext context, Object children, CodedOutputStream codedOut)
       throws IOException, SerializationException {
-    childCodedOut.writeInt32NoTag(1);
+    codedOut.writeInt32NoTag(1);
     T singleChild = cast(children);
-    context.serialize(singleChild, childCodedOut);
+    context.serialize(singleChild, codedOut);
   }
 
   private Object deserializeOneNestedSet(
@@ -158,22 +158,23 @@ public class NestedSetCodec<T> implements ObjectCodec<NestedSet<T>> {
       CodedInputStream codedIn,
       Map<ByteString, Object> digestToChild)
       throws SerializationException, IOException {
-    ByteString digest = codedIn.readBytes();
-    CodedInputStream childCodedIn = codedIn.readBytes().newCodedInput();
-    childCodedIn.enableAliasing(true); // Allow efficient views of byte slices when reading digests
-    int childCount = childCodedIn.readInt32();
+    int childCount = codedIn.readInt32();
     final Object result;
     if (childCount > 1) {
+      ByteString digest = codedIn.readBytes();
+      CodedInputStream childCodedIn = codedIn.readBytes().newCodedInput();
+      childCodedIn.enableAliasing(
+          true); // Allow efficient views of byte slices when reading digests
       result = deserializeMultipleItemChildArray(context, childCodedIn, childCount, digestToChild);
+      digestToChild.put(
+          // Copy the ByteString to avoid keeping the full buffer from codedIn alive due to
+          // aliasing.
+          ByteString.copyFrom(digest.asReadOnlyByteBuffer()), result);
     } else if (childCount == 1) {
-      result = context.deserialize(childCodedIn);
+      result = context.deserialize(codedIn);
     } else {
       result = NestedSet.EMPTY_CHILDREN;
     }
-    digestToChild.put(
-        // Copy the ByteString to avoid keeping the full buffer from codedIn alive due to
-        // aliasing.
-        ByteString.copyFrom(digest.asReadOnlyByteBuffer()), result);
     return result;
   }