Store reverse deps of size 2-4 as a SkyKey[] instead of an `ArrayLi…

…st`.

Most nodes have a small number of reverse deps. There is already an optimization to store a single reverse dep bare. This change extends the optimization to forego an `ArrayList` wrapper (24 bytes) when there are 2-4 elements.

Additionally, this saves space that was being wasted on over-allocating the size of the backing array by calling `Lists.newArrayListWithExpectedSize(2)`, which per the Javadoc on that method, adds "an unspecified amount of padding" (also, the Javadoc discourages using the method altogether). The actual calculation is `5L + arraySize + (arraySize / 10)`, which means for an input of 2 we were allocating an array of size 7.

PiperOrigin-RevId: 540717655
Change-Id: I8d485f8ca463a4178e2844491928748d5f6c182c

src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java

@@ -330,12 +330,7 @@ public DependencyState addReverseDepAndCheckIfDone(SkyKey reverseDep) {
   }
 
   /** Sets {@link #reverseDeps}. Does not alter {@link #reverseDepsDataToConsolidate}. */
-  synchronized void setSingleReverseDepForReverseDepsUtil(SkyKey reverseDep) {
-    this.reverseDeps = reverseDep;
-  }
-
-  /** Sets {@link #reverseDeps}. Does not alter {@link #reverseDepsDataToConsolidate}. */
-  synchronized void setReverseDepsForReverseDepsUtil(List<SkyKey> reverseDeps) {
+  synchronized void setReverseDepsForReverseDepsUtil(Object reverseDeps) {
     this.reverseDeps = reverseDeps;
   }
 

src/main/java/com/google/devtools/build/skyframe/ReverseDepsUtility.java

@@ -19,25 +19,19 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet;
 import com.google.devtools.build.skyframe.KeyToConsolidate.Op;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
 /**
- * A utility class that allows us to keep the reverse dependencies as an array list instead of a
- * set. This is more memory-efficient. At the same time it allows us to group the removals and
- * uniqueness checks so that it also performs well.
- *
- * <p>We could simply make {@link InMemoryNodeEntry} extend this class, but we would be less
- * memory-efficient since object memory alignment does not cross classes (you would have two memory
- * alignments, one for the base class and one for the extended one). We could also merge this
- * functionality directly into {@link InMemoryNodeEntry} at the cost of increasing its size and
- * complexity even more.
+ * A utility class that allows us to store reverse dependencies in a memory-efficient way. At the
+ * same time it allows us to group the removals and uniqueness checks so that it also performs well.
  *
  * <p>The operations {@link #addReverseDep}, {@link #checkReverseDep}, and {@link #removeReverseDep}
  * here are optimized for a done entry. Done entries rarely have rdeps added and removed, but do
@@ -48,6 +42,18 @@
  * <p>{@link InMemoryNodeEntry} manages pending reverse dep operations on a marked-dirty or
  * initially evaluating node itself, using similar logic tuned to those cases, and calls into {@link
  * #consolidateDataAndReturnNewElements} when transitioning to done.
+ *
+ * <p>The storage schema for reverse dependencies of done node entries is:
+ *
+ * <ul>
+ *   <li>0 rdeps: empty {@link ImmutableList}
+ *   <li>1 rdep: bare {@link SkyKey}
+ *   <li>2-4 rdeps: {@code SkyKey[]} (no nulls)
+ *   <li>5+ rdeps: an {@link ArrayList}
+ * </ul>
+ *
+ * This strategy saves memory in the common case of few reverse deps while still supporting constant
+ * time additions for nodes with many rdeps by dynamically switching to an {@link ArrayList}.
  */
 abstract class ReverseDepsUtility {
 
@@ -68,7 +74,7 @@ static Op getOpToStoreBare(InMemoryNodeEntry entry) {
 
   private static void maybeDelayReverseDepOp(InMemoryNodeEntry entry, SkyKey reverseDep, Op op) {
     List<Object> consolidations = entry.getReverseDepsDataToConsolidateForReverseDepsUtil();
-    int currentReverseDepSize = getCurrentReverseDepSize(entry);
+    int currentReverseDepSize = sizeOf(entry.getReverseDepsRawForReverseDepsUtil());
     if (consolidations == null) {
       consolidations = new ArrayList<>(currentReverseDepSize);
       entry.setReverseDepsDataToConsolidateForReverseDepsUtil(consolidations);
@@ -80,47 +86,49 @@ private static void maybeDelayReverseDepOp(InMemoryNodeEntry entry, SkyKey rever
     }
   }
 
-  private static boolean isSingleReverseDep(InMemoryNodeEntry entry) {
-    return !(entry.getReverseDepsRawForReverseDepsUtil() instanceof List);
+  private static boolean isSingleReverseDep(Object raw) {
+    return raw instanceof SkyKey;
   }
 
-  @SuppressWarnings("unchecked") // Cast to list.
-  private static int getCurrentReverseDepSize(InMemoryNodeEntry entry) {
-    return isSingleReverseDep(entry)
-        ? 1
-        : ((List<SkyKey>) entry.getReverseDepsRawForReverseDepsUtil()).size();
+  @SuppressWarnings("unchecked")
+  private static List<SkyKey> multipleAsList(Object raw) {
+    return raw instanceof SkyKey[] ? Arrays.asList((SkyKey[]) raw) : (List<SkyKey>) raw;
   }
 
-  /**
-   * We use a memory-efficient trick to keep reverseDeps memory usage low. Edges in Bazel are
-   * dominant over the number of nodes.
-   *
-   * <p>Most of the nodes have zero or one reverse dep. That is why we use immutable versions of the
-   * lists for those cases. In case of the size being > 1 we switch to an ArrayList. That is because
-   * we also have a decent number of nodes for which the reverseDeps are huge (for example almost
-   * everything depends on BuildInfo node).
-   *
-   * <p>We also optimize for the case where we have only one dependency. In that case we keep the
-   * object directly instead of a wrapper list.
-   */
-  @SuppressWarnings("unchecked") // Cast to SkyKey and List.
+  private static int sizeOf(Object raw) {
+    if (isSingleReverseDep(raw)) {
+      return 1;
+    }
+    if (raw instanceof SkyKey[]) {
+      return ((SkyKey[]) raw).length;
+    }
+    return ((List<?>) raw).size();
+  }
+
+  @SuppressWarnings("unchecked") // Cast to List<SkyKey>.
   static void addReverseDep(InMemoryNodeEntry entry, SkyKey newReverseDep) {
     List<Object> dataToConsolidate = entry.getReverseDepsDataToConsolidateForReverseDepsUtil();
     if (dataToConsolidate != null) {
       maybeDelayReverseDepOp(entry, newReverseDep, Op.ADD);
       return;
     }
-    Object reverseDeps = entry.getReverseDepsRawForReverseDepsUtil();
-    int reverseDepsSize = isSingleReverseDep(entry) ? 1 : ((List<SkyKey>) reverseDeps).size();
-    if (reverseDepsSize == 0) {
-      entry.setSingleReverseDepForReverseDepsUtil(newReverseDep);
-    } else if (reverseDepsSize == 1) {
-      List<SkyKey> newList = Lists.newArrayListWithExpectedSize(2);
-      newList.add((SkyKey) reverseDeps);
+    var raw = entry.getReverseDepsRawForReverseDepsUtil();
+    int newSize = sizeOf(raw) + 1;
+    if (newSize == 1) {
+      entry.setReverseDepsForReverseDepsUtil(newReverseDep);
+    } else if (newSize == 2) {
+      entry.setReverseDepsForReverseDepsUtil(new SkyKey[] {(SkyKey) raw, newReverseDep});
+    } else if (newSize <= 4) {
+      SkyKey[] newArray = Arrays.copyOf((SkyKey[]) raw, newSize);
+      newArray[newSize - 1] = newReverseDep;
+      entry.setReverseDepsForReverseDepsUtil(newArray);
+    } else if (newSize == 5) {
+      List<SkyKey> newList = new ArrayList<>(8);
+      Collections.addAll(newList, (SkyKey[]) raw);
       newList.add(newReverseDep);
       entry.setReverseDepsForReverseDepsUtil(newList);
     } else {
-      ((List<SkyKey>) reverseDeps).add(newReverseDep);
+      ((List<SkyKey>) raw).add(newReverseDep);
     }
   }
 
@@ -147,11 +155,11 @@ static ImmutableCollection<SkyKey> getReverseDeps(
     // TODO(bazel-team): Unfortunately, we need to make a copy here right now to be on the safe side
     // wrt. thread-safety. The parents of a node get modified when any of the parents is deleted,
     // and we can't handle that right now.
-    if (isSingleReverseDep(entry)) {
-      return ImmutableSet.of((SkyKey) entry.getReverseDepsRawForReverseDepsUtil());
+    var raw = entry.getReverseDepsRawForReverseDepsUtil();
+    if (isSingleReverseDep(raw)) {
+      return ImmutableSet.of((SkyKey) raw);
     } else {
-      @SuppressWarnings("unchecked")
-      List<SkyKey> reverseDeps = (List<SkyKey>) entry.getReverseDepsRawForReverseDepsUtil();
+      List<SkyKey> reverseDeps = multipleAsList(raw);
       if (!checkConsistency) {
         return ImmutableList.copyOf(reverseDeps);
       }
@@ -169,7 +177,6 @@ static Set<SkyKey> returnNewElements(InMemoryNodeEntry entry) {
     return consolidateDataAndReturnNewElements(entry, /*mutateObject=*/ false);
   }
 
-  @SuppressWarnings("unchecked") // List and bare SkyKey casts.
   private static Set<SkyKey> consolidateDataAndReturnNewElements(
       InMemoryNodeEntry entry, boolean mutateObject) {
     List<Object> dataToConsolidate = entry.getReverseDepsDataToConsolidateForReverseDepsUtil();
@@ -183,13 +190,13 @@ private static Set<SkyKey> consolidateDataAndReturnNewElements(
     boolean allRdepsAreNew = opToStoreBare == Op.ADD;
     Set<SkyKey> allReverseDeps;
     Set<SkyKey> newReverseDeps;
-    Object reverseDeps = entry.getReverseDepsRawForReverseDepsUtil();
-    if (isSingleReverseDep(entry)) {
+    var raw = entry.getReverseDepsRawForReverseDepsUtil();
+    if (isSingleReverseDep(raw)) {
       Preconditions.checkState(!allRdepsAreNew, entry);
-      allReverseDeps = CompactHashSet.create((SkyKey) reverseDeps);
+      allReverseDeps = CompactHashSet.create((SkyKey) raw);
       newReverseDeps = CompactHashSet.create();
     } else {
-      List<SkyKey> reverseDepsAsList = (List<SkyKey>) reverseDeps;
+      List<SkyKey> reverseDepsAsList = multipleAsList(raw);
       if (allRdepsAreNew) {
         Preconditions.checkState(reverseDepsAsList.isEmpty(), entry);
         allReverseDeps = null;
@@ -261,15 +268,15 @@ private static Set<SkyKey> consolidateDataAndReturnNewElements(
                 allReverseDeps.add(key),
                 "Duplicate reverse deps: %s %s %s %s",
                 keyToConsolidate,
-                reverseDeps,
+                raw,
                 dataToConsolidate,
                 entry);
           }
           Preconditions.checkState(
               newReverseDeps.add(key),
               "Duplicate new reverse deps: %s %s %s %s",
               keyToConsolidate,
-              reverseDeps,
+              raw,
               dataToConsolidate,
               entry);
           break;
@@ -286,20 +293,19 @@ static Set<SkyKey> consolidateDataAndReturnNewElements(InMemoryNodeEntry entry)
     return consolidateDataAndReturnNewElements(entry, /*mutateObject=*/ true);
   }
 
-  @SuppressWarnings("unchecked") // Casts to SkyKey and List.
   private static void consolidateData(InMemoryNodeEntry entry) {
     List<Object> dataToConsolidate = entry.getReverseDepsDataToConsolidateForReverseDepsUtil();
     if (dataToConsolidate == null) {
       return;
     }
     entry.setReverseDepsDataToConsolidateForReverseDepsUtil(null);
-    Object reverseDeps = entry.getReverseDepsRawForReverseDepsUtil();
-    if (isSingleReverseDep(entry)) {
+    var raw = entry.getReverseDepsRawForReverseDepsUtil();
+    if (isSingleReverseDep(raw)) {
       Preconditions.checkState(
           dataToConsolidate.size() == 1,
           "dataToConsolidate not size 1 even though only one rdep: %s %s %s",
           dataToConsolidate,
-          reverseDeps,
+          raw,
           entry);
       Object keyToConsolidate = Iterables.getOnlyElement(dataToConsolidate);
       SkyKey key = KeyToConsolidate.key(keyToConsolidate);
@@ -309,31 +315,28 @@ private static void consolidateData(InMemoryNodeEntry entry) {
           entry.setReverseDepsForReverseDepsUtil(ImmutableList.of());
           // Fall through to check.
         case CHECK:
-          Preconditions.checkState(
-              key.equals(reverseDeps), "%s %s %s", keyToConsolidate, reverseDeps, entry);
+          Preconditions.checkState(key.equals(raw), "%s %s %s", keyToConsolidate, raw, entry);
           break;
         case ADD:
           throw new IllegalStateException(
               "Shouldn't delay add if only one element: "
                   + keyToConsolidate
                   + ", "
-                  + reverseDeps
+                  + raw
                   + ", "
                   + entry);
         case REMOVE_OLD:
           throw new IllegalStateException(
               "Shouldn't be removing old deps if node already done: "
                   + keyToConsolidate
                   + ", "
-                  + reverseDeps
+                  + raw
                   + ", "
                   + entry);
-        default:
-          throw new IllegalStateException(keyToConsolidate + ", " + reverseDeps + ", " + entry);
       }
       return;
     }
-    List<SkyKey> reverseDepsAsList = (List<SkyKey>) reverseDeps;
+    List<SkyKey> reverseDepsAsList = multipleAsList(raw);
     Set<SkyKey> reverseDepsAsSet = getReverseDepsSet(entry, reverseDepsAsList);
 
     Preconditions.checkState(getOpToStoreBare(entry) == Op.CHECK, entry);
@@ -354,7 +357,7 @@ private static void consolidateData(InMemoryNodeEntry entry) {
               reverseDepsAsSet.remove(key),
               "%s %s %s %s",
               keyToConsolidate,
-              reverseDeps,
+              raw,
               dataToConsolidate,
               entry);
           break;
@@ -363,7 +366,7 @@ private static void consolidateData(InMemoryNodeEntry entry) {
               reverseDepsAsSet.add(key),
               "%s %s %s %s",
               keyToConsolidate,
-              reverseDeps,
+              raw,
               dataToConsolidate,
               entry);
           break;
@@ -372,14 +375,11 @@ private static void consolidateData(InMemoryNodeEntry entry) {
               "Shouldn't be removing old deps if node already done: "
                   + keyToConsolidate
                   + ", "
-                  + reverseDeps
+                  + raw
                   + ", "
                   + dataToConsolidate
                   + ", "
                   + entry);
-        default:
-          throw new IllegalStateException(
-              keyToConsolidate + ", " + reverseDepsAsSet + ", " + dataToConsolidate + ", " + entry);
       }
     }
     writeReverseDepsSet(entry, reverseDepsAsSet);
@@ -389,7 +389,9 @@ private static void writeReverseDepsSet(InMemoryNodeEntry entry, Set<SkyKey> rev
     if (!entry.keepsEdges() || reverseDepsAsSet.isEmpty()) {
       entry.setReverseDepsForReverseDepsUtil(ImmutableList.of());
     } else if (reverseDepsAsSet.size() == 1) {
-      entry.setSingleReverseDepForReverseDepsUtil(Iterables.getOnlyElement(reverseDepsAsSet));
+      entry.setReverseDepsForReverseDepsUtil(Iterables.getOnlyElement(reverseDepsAsSet));
+    } else if (reverseDepsAsSet.size() <= 4) {
+      entry.setReverseDepsForReverseDepsUtil(reverseDepsAsSet.toArray(SkyKey[]::new));
     } else {
       entry.setReverseDepsForReverseDepsUtil(new ArrayList<>(reverseDepsAsSet));
     }