From ae6b3a56e28d73a2f34e5754d8a3ad5999da35f8 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 15 Jun 2023 16:09:47 -0700 Subject: [PATCH] Store reverse deps of size 2-4 as a `SkyKey[]` instead of an `ArrayList`. 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 --- .../build/skyframe/InMemoryNodeEntry.java | 7 +- .../build/skyframe/ReverseDepsUtility.java | 138 +++++++++--------- 2 files changed, 71 insertions(+), 74 deletions(-) diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java index fb246ac5ccc785..dbb3b3a773737f 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java +++ b/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 reverseDeps) { + synchronized void setReverseDepsForReverseDepsUtil(Object reverseDeps) { this.reverseDeps = reverseDeps; } diff --git a/src/main/java/com/google/devtools/build/skyframe/ReverseDepsUtility.java b/src/main/java/com/google/devtools/build/skyframe/ReverseDepsUtility.java index 60ece94d019f30..65e9234063ce4f 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ReverseDepsUtility.java +++ b/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. - * - *

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. * *

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 @@ *

{@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. + * + *

The storage schema for reverse dependencies of done node entries is: + * + *

+ * + * 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 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) entry.getReverseDepsRawForReverseDepsUtil()).size(); + @SuppressWarnings("unchecked") + private static List multipleAsList(Object raw) { + return raw instanceof SkyKey[] ? Arrays.asList((SkyKey[]) raw) : (List) raw; } - /** - * We use a memory-efficient trick to keep reverseDeps memory usage low. Edges in Bazel are - * dominant over the number of nodes. - * - *

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). - * - *

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. static void addReverseDep(InMemoryNodeEntry entry, SkyKey newReverseDep) { List dataToConsolidate = entry.getReverseDepsDataToConsolidateForReverseDepsUtil(); if (dataToConsolidate != null) { maybeDelayReverseDepOp(entry, newReverseDep, Op.ADD); return; } - Object reverseDeps = entry.getReverseDepsRawForReverseDepsUtil(); - int reverseDepsSize = isSingleReverseDep(entry) ? 1 : ((List) reverseDeps).size(); - if (reverseDepsSize == 0) { - entry.setSingleReverseDepForReverseDepsUtil(newReverseDep); - } else if (reverseDepsSize == 1) { - List 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 newList = new ArrayList<>(8); + Collections.addAll(newList, (SkyKey[]) raw); newList.add(newReverseDep); entry.setReverseDepsForReverseDepsUtil(newList); } else { - ((List) reverseDeps).add(newReverseDep); + ((List) raw).add(newReverseDep); } } @@ -147,11 +155,11 @@ static ImmutableCollection 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 reverseDeps = (List) entry.getReverseDepsRawForReverseDepsUtil(); + List reverseDeps = multipleAsList(raw); if (!checkConsistency) { return ImmutableList.copyOf(reverseDeps); } @@ -169,7 +177,6 @@ static Set returnNewElements(InMemoryNodeEntry entry) { return consolidateDataAndReturnNewElements(entry, /*mutateObject=*/ false); } - @SuppressWarnings("unchecked") // List and bare SkyKey casts. private static Set consolidateDataAndReturnNewElements( InMemoryNodeEntry entry, boolean mutateObject) { List dataToConsolidate = entry.getReverseDepsDataToConsolidateForReverseDepsUtil(); @@ -183,13 +190,13 @@ private static Set consolidateDataAndReturnNewElements( boolean allRdepsAreNew = opToStoreBare == Op.ADD; Set allReverseDeps; Set 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 reverseDepsAsList = (List) reverseDeps; + List reverseDepsAsList = multipleAsList(raw); if (allRdepsAreNew) { Preconditions.checkState(reverseDepsAsList.isEmpty(), entry); allReverseDeps = null; @@ -261,7 +268,7 @@ private static Set consolidateDataAndReturnNewElements( allReverseDeps.add(key), "Duplicate reverse deps: %s %s %s %s", keyToConsolidate, - reverseDeps, + raw, dataToConsolidate, entry); } @@ -269,7 +276,7 @@ private static Set consolidateDataAndReturnNewElements( newReverseDeps.add(key), "Duplicate new reverse deps: %s %s %s %s", keyToConsolidate, - reverseDeps, + raw, dataToConsolidate, entry); break; @@ -286,20 +293,19 @@ static Set consolidateDataAndReturnNewElements(InMemoryNodeEntry entry) return consolidateDataAndReturnNewElements(entry, /*mutateObject=*/ true); } - @SuppressWarnings("unchecked") // Casts to SkyKey and List. private static void consolidateData(InMemoryNodeEntry entry) { List 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,15 +315,14 @@ 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: @@ -325,15 +330,13 @@ private static void consolidateData(InMemoryNodeEntry entry) { "Shouldn't be removing old deps if node already done: " + keyToConsolidate + ", " - + reverseDeps + + raw + ", " + entry); - default: - throw new IllegalStateException(keyToConsolidate + ", " + reverseDeps + ", " + entry); } return; } - List reverseDepsAsList = (List) reverseDeps; + List reverseDepsAsList = multipleAsList(raw); Set 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 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)); }