Move signaledDeps to DirtyBuildingState

This requires using DirtyBuildingState also for the initial evaluation
of a node. As a side effect, this reduces the set of possible states,
which might make it easier to reason about the state.

States: previously, NodeEntry distinguished between initial evaluation
and incremental update, with 2 different states (just created / marked
and evaluating) for each case. With this change, the case distinction
goes away.

This subtly changes the semantics of isDirty(): previously, isDirty
returned false for new nodes; now it returns true for new nodes after
their computation is triggered.

This change should reduce memory consumption of InMemoryNodeEntry
objects per instance for nodes that are done, but at the expense of
additional temporary DirtyBuildingState instances for nodes that are
not done.

I am making this change in preparation for adding another field to the
dirty state, which would otherwise increase memory consumption of all
done nodes.

= Memory consumption
While this decreases the memory consumption of Skyframe nodes in the
done state, it also increases the memory consumption of Skyframe nodes
that have to be recomputed, especially for new nodes, but also for
incrementally updated nodes.

New nodes now have to allocate a DirtyBuildingState instance while they
are in-flight, and incrementally updated nodes now have an additional
field in the DirtyBuildingState. For incrementally updated nodes, the
difference is a wash - we save some in InMemoryNodeEntry and lose the
same amount in DirtyBuildingState.

For clean builds, whether or not this is a reduction in memory use
depends on the ratio of in-flight to done nodes. The higher the ratio,
the higher the memory consumption. Note that the ratio goes to zero at
the end of a build, so this is a strict win for memory consumption at
rest (i.e., between builds), and only a concern while Bazel is active.

For some representative target, I saw ratios of less than 25% at max
during the loading+analysis phase, and dropping off sharply as packages
were completed. During the execution phase, the ratio only barely got
above 10% and usually stayed below 5%.

Assuming a memory difference of 8 bytes per done node (since the Jvm
performs 8-byte alignment), and an increase of 24 bytes per in-flight
node, any ratio below 1/3 is a net win for post-gc memory consumption,
although at the cost of slightly higher gc pressure. This is clearly the
case from the numbers I collected.

For incremental builds, the number of invalidated and recomputed nodes
is usually much, much lower (I saw numbers as low as 0.02%), which
virtually guarantees that this change is a net win.

PiperOrigin-RevId: 233899818

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

@@ -671,19 +671,18 @@ static void injectValues(
       Preconditions.checkState(
           newState != DependencyState.ALREADY_EVALUATING, "%s %s", key, prevEntry);
       if (prevEntry.isDirty()) {
+        // Get the node in the state where it is able to accept a value.
         Preconditions.checkState(
             newState == DependencyState.NEEDS_SCHEDULING, "%s %s", key, prevEntry);
-        // There was an existing entry for this key in the graph.
-        // Get the node in the state where it is able to accept a value.
-
-        // Check that the previous node has no dependencies. Overwriting a value with deps with an
-        // injected value (which is by definition deps-free) needs a little additional bookkeeping
-        // (removing reverse deps from the dependencies), but more importantly it's something that
-        // we want to avoid, because it indicates confusion of input values and derived values.
+        // If there was a node in the graph before, check that the previous node has no
+        // dependencies. Overwriting a value with deps with an injected value (which is by
+        // definition deps-free) needs a little additional bookkeeping (removing reverse deps from
+        // the dependencies), but more importantly it's something that we want to avoid, because it
+        // indicates confusion of input values and derived values.
         Preconditions.checkState(
             prevEntry.noDepsLastBuild(), "existing entry for %s has deps: %s", key, prevEntry);
-        prevEntry.markRebuilding();
       }
+      prevEntry.markRebuilding();
       prevEntry.setValue(
           value,
           version,

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

@@ -199,9 +199,6 @@ private boolean invalidatedByErrorTransience(Collection<SkyKey> depGroup, NodeEn
     }
 
     private DirtyOutcome maybeHandleDirtyNode(NodeEntry state) throws InterruptedException {
-      if (!state.isDirty()) {
-        return DirtyOutcome.NEEDS_EVALUATION;
-      }
       while (state.getDirtyState().equals(DirtyState.CHECK_DEPENDENCIES)) {
         // Evaluating a dirty node for the first time, and checking its children to see if any
         // of them have changed. Note that there must be dirty children for this to happen.

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

@@ -21,6 +21,7 @@
 import com.google.devtools.build.skyframe.ThinNodeEntry.DirtyType;
 import java.util.List;
 import java.util.Set;
+import javax.annotation.Nullable;
 
 /**
  * State for a node that has been dirtied, and will be checked to see if it needs re-evaluation, and
@@ -30,6 +31,17 @@
  * package.
  */
 public abstract class DirtyBuildingState {
+  private static final int NOT_EVALUATING_SENTINEL = -1;
+
+  static DirtyBuildingState create(
+      DirtyType dirtyType, GroupedList<SkyKey> lastBuildDirectDeps, SkyValue lastBuildValue) {
+    return new FullDirtyBuildingState(dirtyType, lastBuildDirectDeps, lastBuildValue);
+  }
+
+  static DirtyBuildingState createNew() {
+    return new FullDirtyBuildingState(DirtyType.CHANGE, null, null);
+  }
+
   /**
    * The state of a dirty node. A node is marked dirty in the DirtyBuildingState constructor, and
    * goes into either the state {@link DirtyState#CHECK_DEPENDENCIES} or {@link
@@ -38,6 +50,29 @@ public abstract class DirtyBuildingState {
    */
   private DirtyState dirtyState;
 
+  /**
+   * The number of dependencies that are known to be done in a {@link NodeEntry}.
+   *
+   * <p>There is a potential check-then-act race here during evaluation, so we need to make sure
+   * that when this is increased, we always check if the new value is equal to the number of
+   * required dependencies, and if so, we must re-schedule the node for evaluation.
+   *
+   * <p>There are two potential pitfalls here: 1) If multiple dependencies signal this node in close
+   * succession, this node should be scheduled exactly once. 2) If a thread is still working on this
+   * node, it should not be scheduled.
+   *
+   * <p>To solve the first problem, the {@link NodeEntry#signalDep} method also returns if the node
+   * needs to be re-scheduled, and ensures that only one thread gets a true return value.
+   *
+   * <p>The second problem is solved by first adding the newly discovered deps to a node's {@link
+   * InMemoryNodeEntry#directDeps}, and then looping through the direct deps and registering this
+   * node as a reverse dependency. This ensures that the signaledDeps counter can only reach {@link
+   * InMemoryNodeEntry#directDeps#numElements} on the very last iteration of the loop, i.e., the
+   * thread is not working on the node anymore. Note that this requires that there is no code after
+   * the loop in {@link ParallelEvaluator.Evaluate#run}.
+   */
+  private int signaledDeps = NOT_EVALUATING_SENTINEL;
+
   /**
    * The dependencies requested (with group markers) last time the node was built (and below, the
    * value last time the node was built). They will be compared to dependencies requested on this
@@ -47,6 +82,7 @@ public abstract class DirtyBuildingState {
    *
    * <p>Public only for the use of alternative graph implementations.
    */
+  @Nullable
   public abstract GroupedList<SkyKey> getLastBuildDirectDeps() throws InterruptedException;
 
   /**
@@ -64,12 +100,13 @@ public abstract class DirtyBuildingState {
    *
    * <p>Public only for the use of alternative graph implementations.
    */
+  @Nullable
   public abstract SkyValue getLastBuildValue() throws InterruptedException;
 
   /**
    * Group of children to be checked next in the process of determining if this entry needs to be
    * re-evaluated. Used by {@link DirtyBuildingState#getNextDirtyDirectDeps} and {@link
-   * #signalDepInternal}.
+   * #signalDepPostProcess}.
    */
   protected int dirtyDirectDepIndex;
 
@@ -80,10 +117,8 @@ protected DirtyBuildingState(DirtyType dirtyType) {
     dirtyDirectDepIndex = 0;
   }
 
-  static DirtyBuildingState create(
-      DirtyType dirtyType, GroupedList<SkyKey> lastBuildDirectDeps, SkyValue lastBuildValue) {
-    return new FullDirtyBuildingState(dirtyType, lastBuildDirectDeps, lastBuildValue);
-  }
+  /** Returns true if this state does have information about a previously built version. */
+  protected abstract boolean isDirty();
 
   final void markChanged() {
     Preconditions.checkState(dirtyState == DirtyState.CHECK_DEPENDENCIES, this);
@@ -97,7 +132,8 @@ final void markForceRebuild() {
     }
   }
 
-  final void forceRebuild() {
+  final void forceRebuild(int numTemporaryDirectDeps) {
+    Preconditions.checkState(numTemporaryDirectDeps == signaledDeps, this);
     Preconditions.checkState(
         (dirtyState == DirtyState.CHECK_DEPENDENCIES
                 && getNumOfGroupsInLastBuildDirectDeps() == dirtyDirectDepIndex)
@@ -106,6 +142,10 @@ && getNumOfGroupsInLastBuildDirectDeps() == dirtyDirectDepIndex)
     dirtyState = DirtyState.FORCED_REBUILDING;
   }
 
+  final boolean isEvaluating() {
+    return signaledDeps > NOT_EVALUATING_SENTINEL;
+  }
+
   final boolean isChanged() {
     return dirtyState == DirtyState.NEEDS_REBUILDING
         || dirtyState == DirtyState.NEEDS_FORCED_REBUILDING
@@ -122,14 +162,19 @@ private void checkFinishedBuildingWhenAboutToSetValue() {
         this);
   }
 
+  final void signalDep() {
+    Preconditions.checkState(isEvaluating());
+    signaledDeps++;
+  }
+
   /**
    * If this node is not yet known to need rebuilding, sets {@link #dirtyState} to {@link
    * DirtyState#NEEDS_REBUILDING} if the child has changed, and {@link DirtyState#VERIFIED_CLEAN} if
    * the child has not changed and this was the last child to be checked (as determined by {@code
    * isReady} and comparing {@link #dirtyDirectDepIndex} and {@link
    * DirtyBuildingState#getNumOfGroupsInLastBuildDirectDeps()}.
    */
-  final void signalDepInternal(boolean childChanged, boolean isReady) {
+  final void signalDepPostProcess(boolean childChanged, int numTemporaryDirectDeps) {
     Preconditions.checkState(
         isChanged() || (dirtyState == DirtyState.CHECK_DEPENDENCIES && dirtyDirectDepIndex > 0),
         "Unexpected not evaluating: %s",
@@ -140,7 +185,7 @@ final void signalDepInternal(boolean childChanged, boolean isReady) {
       if (childChanged) {
         dirtyState = DirtyState.NEEDS_REBUILDING;
       } else if (dirtyState == DirtyState.CHECK_DEPENDENCIES
-          && isReady
+          && isReady(numTemporaryDirectDeps)
           && getNumOfGroupsInLastBuildDirectDeps() == dirtyDirectDepIndex) {
         // No other dep already marked this as NEEDS_REBUILDING, no deps outstanding, and this was
         // the last block of deps to be checked.
@@ -167,7 +212,9 @@ public final void unmarkNeedsRebuilding() {
    */
   public final boolean unchangedFromLastBuild(SkyValue newValue) throws InterruptedException {
     checkFinishedBuildingWhenAboutToSetValue();
-    return !(newValue instanceof NotComparableSkyValue) && getLastBuildValue().equals(newValue);
+    return !(newValue instanceof NotComparableSkyValue)
+        && getLastBuildValue() != null
+        && getLastBuildValue().equals(newValue);
   }
 
   /**
@@ -206,8 +253,10 @@ final List<SkyKey> getNextDirtyDirectDeps() throws InterruptedException {
    * process the returned set, and so subsequent calls to this method will return the empty set.
    */
   Set<SkyKey> getAllRemainingDirtyDirectDeps(boolean preservePosition) throws InterruptedException {
+    if (getLastBuildDirectDeps() == null) {
+      return ImmutableSet.of();
+    }
     ImmutableSet.Builder<SkyKey> result = ImmutableSet.builder();
-
     for (int ind = dirtyDirectDepIndex; ind < getNumOfGroupsInLastBuildDirectDeps(); ind++) {
       result.addAll(getLastBuildDirectDeps().get(ind));
     }
@@ -220,6 +269,7 @@ Set<SkyKey> getAllRemainingDirtyDirectDeps(boolean preservePosition) throws Inte
   final void resetForRestartFromScratch() {
     Preconditions.checkState(
         dirtyState == DirtyState.REBUILDING || dirtyState == DirtyState.FORCED_REBUILDING, this);
+    signaledDeps = 0;
     dirtyDirectDepIndex = 0;
   }
 
@@ -228,13 +278,29 @@ protected void markRebuilding(boolean isEligibleForChangePruning) {
     dirtyState = DirtyState.REBUILDING;
   }
 
+  void startEvaluating() {
+    Preconditions.checkState(!isEvaluating(), this);
+    signaledDeps = 0;
+  }
+
   public int getLastDirtyDirectDepIndex() {
     return dirtyDirectDepIndex - 1;
   }
 
+  public int getSignaledDeps() {
+    return signaledDeps;
+  }
+
+  /** Returns whether all known children of this node have signaled that they are done. */
+  boolean isReady(int numDirectDeps) {
+    Preconditions.checkState(signaledDeps <= numDirectDeps, "%s %s", numDirectDeps, this);
+    return signaledDeps == numDirectDeps;
+  }
+
   protected MoreObjects.ToStringHelper getStringHelper() {
     return MoreObjects.toStringHelper(this)
         .add("dirtyState", dirtyState)
+        .add("signaledDeps", signaledDeps)
         .add("dirtyDirectDepIndex", dirtyDirectDepIndex);
   }
 
@@ -258,6 +324,11 @@ private FullDirtyBuildingState(
       this.lastBuildValue = lastBuildValue;
     }
 
+    @Override
+    protected boolean isDirty() {
+      return lastBuildDirectDeps != null;
+    }
+
     @Override
     public SkyValue getLastBuildValue() {
       return lastBuildValue;
@@ -270,7 +341,7 @@ public GroupedList<SkyKey> getLastBuildDirectDeps() throws InterruptedException
 
     @Override
     protected int getNumOfGroupsInLastBuildDirectDeps() {
-      return lastBuildDirectDeps.listSize();
+      return lastBuildDirectDeps == null ? 0 : lastBuildDirectDeps.listSize();
     }
 
     @Override