commit dd1235f1048c473ca84fe981f27452ce9eb7db1f
author jhorvitz <jhorvitz@google.com> Fri Dec 10 09:39:53 2021 -0800
committer Copybara-Service <copybara-worker@google.com> Fri Dec 10 09:41:00 2021 -0800
tree f2cd5f736efbf41a6632d1468cca1a7d2ac8b797
parent 20fb6e3a6e1cae51baeb61a38fe790ea6c477f6c

Manage discarding of deps and rdeps from `InMemoryNodeEntry` based on the node's `KeepEdgesPolicy` instead of leaving it to an overridable method `postProcessAfterDone`.

Additionally, use the node's `KeepEdgesPolicy` to determine which rdep operation to store bare. This opens up the opportunity for an optimization in `ReverseDepsUtility` - for nodes on their initial build and nodes that do not keep rdeps, we can build one set instead of two because all rdeps are new.

PiperOrigin-RevId: 415541716

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 7084da7..a2efa1d 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -22,7 +22,6 @@
 import com.google.devtools.build.lib.util.GroupedList;
 import com.google.devtools.build.lib.util.GroupedList.GroupedListHelper;
 import com.google.devtools.build.skyframe.KeyToConsolidate.Op;
-import com.google.devtools.build.skyframe.KeyToConsolidate.OpToStoreBare;
 import com.google.errorprone.annotations.ForOverride;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -253,21 +252,16 @@
   }
 
   protected final synchronized Set<SkyKey> setStateFinishedAndReturnReverseDepsToSignal() {
-    Set<SkyKey> reverseDepsToSignal =
-        ReverseDepsUtility.consolidateDataAndReturnNewElements(this, getOpToStoreBare());
-    this.directDeps = getTemporaryDirectDeps().compress();
-
+    Set<SkyKey> reverseDepsToSignal = ReverseDepsUtility.consolidateDataAndReturnNewElements(this);
+    directDeps = keepEdges() == KeepEdgesPolicy.NONE ? null : getTemporaryDirectDeps().compress();
     markDone();
-    postProcessAfterDone();
     return reverseDepsToSignal;
   }
 
-  protected void postProcessAfterDone() {}
-
   @Override
   public synchronized Set<SkyKey> getInProgressReverseDeps() {
     Preconditions.checkState(!isDone(), this);
-    return ReverseDepsUtility.returnNewElements(this, getOpToStoreBare());
+    return ReverseDepsUtility.returnNewElements(this);
   }
 
   /**
@@ -312,6 +306,9 @@
 
     synchronized (this) {
       boolean done = isDone();
+      if (!done && dirtyBuildingState == null) {
+        dirtyBuildingState = DirtyBuildingState.createNew();
+      }
       if (reverseDep != null) {
         if (done) {
           if (keepReverseDeps()) {
@@ -324,9 +321,6 @@
       if (done) {
         return DependencyState.DONE;
       }
-      if (dirtyBuildingState == null) {
-        dirtyBuildingState = DirtyBuildingState.createNew();
-      }
       boolean wasEvaluating = dirtyBuildingState.isEvaluating();
       if (!wasEvaluating) {
         dirtyBuildingState.startEvaluating();
@@ -366,18 +360,7 @@
     }
     Preconditions.checkState(
         isDirty() || op != Op.CHECK, "Not dirty check %s %s", this, reverseDep);
-    reverseDepsDataToConsolidate.add(KeyToConsolidate.create(reverseDep, op, getOpToStoreBare()));
-  }
-
-  /**
-   * In order to reduce memory consumption, we want to store reverse deps 'bare', i.e., without
-   * wrapping them in a KeyToConsolidate object. To that end, we define a bare op that is used for
-   * both storing and retrieving the deps. This method returns said op, and may adjust it depending
-   * on whether this is a new node entry (where all deps must be new) or an existing node entry
-   * (which most likely checks deps rather than adding new deps).
-   */
-  protected OpToStoreBare getOpToStoreBare() {
-    return isDirty() && dirtyBuildingState.isDirty() ? OpToStoreBare.CHECK : OpToStoreBare.ADD;
+    reverseDepsDataToConsolidate.add(KeyToConsolidate.create(reverseDep, op, this));
   }
 
   @Override
@@ -437,7 +420,7 @@
     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());
+      ReverseDepsUtility.consolidateDataAndReturnNewElements(this);
     }
     return ReverseDepsUtility.getReverseDeps(this, /*checkConsistency=*/ false);
   }