commit 3ea75594bfdb5e6794250155995c45a5adc00c28
author janakr <janakr@google.com> Wed Aug 04 09:30:54 2021 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Aug 04 09:32:19 2021 -0700
tree cb4652e78943d2f1623597ce328dabbda08baa30
parent d0d0c5fa8652ee6232f93b133db61d1456034cb2

Optimize some parts of Skyframe node deletion, especially for the case of discarding the analysis cache on a configuration change:

1. Avoid processing to-be-deleted direct deps of a node being deleted: removing this node from the dep's reverse deps is useful only as a consistency check and is too expensive to justify, given that it's been a long time since there was a bug there.

2. To make (1) more effective, in an analysis-cache-discarding event, eagerly mark all ActionLookupData nodes as to-be-deleted: they will be deleted anyway, since they depend on ActionLookupKey nodes, and marking them eagerly means we can apply the optimization in (1).

3. Don't make an ImmutableSet copy of the reverse deps in ReverseDepsUtility#getReverseDeps when the node is about to be deleted: the consistency check is not worth the cost.

This should reduce CPU, memory churn, and contention.

PiperOrigin-RevId: 388715102

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

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 843eba5..d403651 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -23,6 +23,7 @@
 import com.google.devtools.build.skyframe.KeyToConsolidate.OpToStoreBare;
 import com.google.errorprone.annotations.ForOverride;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.Set;
 import javax.annotation.Nullable;
@@ -458,21 +459,21 @@
   }
 
   @Override
-  public synchronized Set<SkyKey> getReverseDepsForDoneEntry() {
+  public synchronized Collection<SkyKey> getReverseDepsForDoneEntry() {
     assertKeepRdeps();
     Preconditions.checkState(isDone(), "Called on not done %s", this);
-    return ReverseDepsUtility.getReverseDeps(this);
+    return ReverseDepsUtility.getReverseDeps(this, /*checkConsistency=*/ true);
   }
 
   @Override
-  public synchronized Set<SkyKey> getAllReverseDepsForNodeBeingDeleted() {
+  public synchronized Collection<SkyKey> getAllReverseDepsForNodeBeingDeleted() {
     assertKeepRdeps();
     if (!isDone()) {
       // This consolidation loses information about pending reverse deps to signal, but that is
       // unimportant since this node is being deleted.
       ReverseDepsUtility.consolidateDataAndReturnNewElements(this, getOpToStoreBare());
     }
-    return ReverseDepsUtility.getReverseDeps(this);
+    return ReverseDepsUtility.getReverseDeps(this, /*checkConsistency=*/ false);
   }
 
   @Override
@@ -525,7 +526,7 @@
       this.directDeps = null;
       return new MarkedDirtyResult(
           KeepEdgesPolicy.ALL.equals(keepEdges())
-              ? ReverseDepsUtility.getReverseDeps(this)
+              ? ReverseDepsUtility.getReverseDeps(this, /*checkConsistency=*/ true)
               : ImmutableList.of());
     }
     if (dirtyType.equals(DirtyType.FORCE_REBUILD)) {
@@ -723,7 +724,7 @@
     newEntry.value = value;
     newEntry.lastChangedVersion = this.lastChangedVersion;
     newEntry.lastEvaluatedVersion = this.lastEvaluatedVersion;
-    for (SkyKey reverseDep : ReverseDepsUtility.getReverseDeps(this)) {
+    for (SkyKey reverseDep : ReverseDepsUtility.getReverseDeps(this, /*checkConsistency=*/ true)) {
       ReverseDepsUtility.addReverseDep(newEntry, reverseDep);
     }
     newEntry.directDeps = directDeps;