Remove BuildingState, since it only has one field. Instead, keep the signaledDeps field directly in InMemoryNodeEntry.

Should save ~24 bytes per freshly evaluating node entry (I haven't calculated object alignment for InMemoryNodeEntry now, so could be more or less). Also might save some memory for re-evaluating node entries, since the BuildingState class had to be padded out to a multiple of 8 bytes before the DirtyBuildingState fields could start. Don't actually know if that was happening.

--
PiperOrigin-RevId: 151138224
MOS_MIGRATED_REVID=151138224

3 files changed, 118 insertions, 236 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/BuildingState.java b/src/main/java/com/google/devtools/build/skyframe/BuildingState.java
deleted file mode 100644
index 455ecda809..0000000000
--- a/src/main/java/com/google/devtools/build/skyframe/BuildingState.java
+++ /dev/null
@@ -1,162 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.skyframe;
-
-import com.google.common.base.MoreObjects;
-import com.google.common.base.MoreObjects.ToStringHelper;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
-import com.google.devtools.build.lib.util.Preconditions;
-
-/**
- * Data the NodeEntry uses to maintain its state before it is done building. It allows the {@link
- * NodeEntry} to keep the current state of the entry across invalidation and successive evaluations.
- * A done node does not contain any of this data. However, if a node is marked dirty, its entry
- * acquires a new {@link DirtyBuildingState} object, which persists until it is done again.
- *
- * <p>This class should be considered a private inner class of {@link InMemoryNodeEntry} -- no other
- * classes should instantiate a {@code BuildingState} object or call any of its methods directly. It
- * is in a separate file solely to keep the {@link NodeEntry} class readable. In particular, the
- * caller must synchronize access to this class.
- *
- * <p>During its life, a node can go through states as follows:
- *
- * <ol>
- * <li>Non-existent
- * <li>Just created ({@link #isEvaluating} is false)
- * <li>Evaluating ({@link #isEvaluating} is true)
- * <li>Done (meaning this buildingState object is null)
- * <li>Just created (when it is dirtied during evaluation)
- * <li>Reset (just before it is re-evaluated)
- * <li>Evaluating
- * <li>Done
- * </ol>
- *
- * <p>The "just created" state is there to allow the {@link EvaluableGraph#createIfAbsentBatch} and
- * {@link NodeEntry#addReverseDepAndCheckIfDone} methods to be separate. All callers have to call
- * both methods in that order if they want to create a node. The second method calls {@link
- * #startEvaluating}, which transitions the current node to the "evaluating" state and returns true
- * only the first time it was called. A caller that gets "true" back from that call must start the
- * evaluation of this node, while any subsequent callers must not.
- *
- * <p>An entry is set to "evaluating" as soon as it is scheduled for evaluation. Thus, even a node
- * that is never actually built (for instance, a dirty node that is verified as clean) is in the
- * "evaluating" state until it is done.
- */
-@ThreadCompatible
-class BuildingState {
-  protected static final int NOT_EVALUATING_SENTINEL = -1;
-
-  /**
-   * The number of dependencies that are known to be done in a {@link NodeEntry} if it is already
-   * evaluating, and a sentinel (-1) indicating that it has not yet started evaluating otherwise.
-   * 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>The first problem is solved by the {@link #signalDep} method, which 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 {@code ParallelEvaluator.Evaluate#run}.
-   */
-  protected int signaledDeps = NOT_EVALUATING_SENTINEL;
-
-  /** Returns whether all known children of this node have signaled that they are done. */
-  final boolean isReady(int numDirectDeps) {
-    Preconditions.checkState(signaledDeps <= numDirectDeps, "%s %s", numDirectDeps, this);
-    return signaledDeps == numDirectDeps;
-  }
-
-  /**
-   * Returns true if the entry is marked dirty, meaning that at least one of its transitive
-   * dependencies is marked changed.
-   *
-   * @see NodeEntry#isDirty()
-   */
-  boolean isDirty() {
-    return false;
-  }
-
-  /**
-   * Returns true if the entry is known to require re-evaluation.
-   *
-   * @see NodeEntry#isChanged()
-   */
-  boolean isChanged() {
-    return false;
-  }
-
-  /**
-   * Helper method to assert that node has finished building, as far as we can tell. We would
-   * actually like to check that the node has been evaluated, but that is not available in this
-   * context.
-   */
-  protected void checkFinishedBuildingWhenAboutToSetValue() {
-    Preconditions.checkState(isEvaluating(), "not started building %s", this);
-    Preconditions.checkState(!isDirty(), "not done building %s", this);
-  }
-
-  /**
-   * Puts the node in the "evaluating" state if it is not already in it. Returns true if the node
-   * wasn't already evaluating and false otherwise. Should only be called by {@link
-   * NodeEntry#addReverseDepAndCheckIfDone}.
-   */
-  final boolean startEvaluating() {
-    boolean result = !isEvaluating();
-    if (result) {
-      signaledDeps = 0;
-    }
-    return result;
-  }
-
-  final boolean isEvaluating() {
-    return signaledDeps > NOT_EVALUATING_SENTINEL;
-  }
-
-  /**
-   * Increments the number of children known to be finished. Returns true if the number of children
-   * finished is equal to the number of known children.
-   *
-   * <p>If the node is dirty and checking its deps for changes, this also updates dirty state as
-   * needed, via {@link #signalDepInternal}.
-   *
-   * @see NodeEntry#signalDep(Version)
-   */
-  final boolean signalDep(boolean childChanged, int numDirectDeps) {
-    Preconditions.checkState(isEvaluating(), this);
-    signaledDeps++;
-    signalDepInternal(childChanged, numDirectDeps);
-    return isReady(numDirectDeps);
-  }
-
-  void signalDepInternal(boolean childChanged, int numDirectDeps) {}
-
-  protected ToStringHelper getStringHelper() {
-    return MoreObjects.toStringHelper(this)
-        .add("hash", System.identityHashCode(this))
-        .add("signaledDeps/evaluating state", signaledDeps);
-  }
-  @Override
-  public final String toString() {
-    return getStringHelper().toString();
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java b/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java
index 2c34feb107..2b8dc9aef6 100644
--- a/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java
+++ b/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java
@@ -22,13 +22,13 @@ import java.util.Collection;
 import java.util.Set;
 
 /**
- * A {@link BuildingState} for a node that has been dirtied, and will be checked to see if it needs
- * re-evaluation, and either marked clean or re-evaluated. See {@link BuildingState} for more.
+ * State for a node that has been dirtied, and will be checked to see if it needs re-evaluation, and
+ * either marked clean or re-evaluated.
  *
  * <p>This class is public only for the benefit of alternative graph implementations outside of the
  * package.
  */
-public abstract class DirtyBuildingState extends BuildingState {
+public abstract class DirtyBuildingState {
   /**
    * 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
@@ -58,9 +58,10 @@ public abstract class DirtyBuildingState extends BuildingState {
 
   /**
    * 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 #signalDep}.
+   * re-evaluated. Used by {@link DirtyBuildingState#getNextDirtyDirectDeps} and {@link
+   * #signalDepInternal}.
    */
-  private int dirtyDirectDepIndex = -1;
+  protected int dirtyDirectDepIndex;
 
   protected DirtyBuildingState(boolean isChanged) {
     dirtyState = isChanged ? DirtyState.NEEDS_REBUILDING : DirtyState.CHECK_DEPENDENCIES;
@@ -69,47 +70,30 @@ public abstract class DirtyBuildingState extends BuildingState {
     dirtyDirectDepIndex = 0;
   }
 
-  static BuildingState create(
+  static DirtyBuildingState create(
       boolean isChanged, GroupedList<SkyKey> lastBuildDirectDeps, SkyValue lastBuildValue) {
     return new FullDirtyBuildingState(isChanged, lastBuildDirectDeps, lastBuildValue);
   }
 
   final void markChanged() {
-    Preconditions.checkState(isDirty(), this);
-    Preconditions.checkState(!isChanged(), this);
-    Preconditions.checkState(!isEvaluating(), this);
+    Preconditions.checkState(dirtyState == DirtyState.CHECK_DEPENDENCIES, this);
+    Preconditions.checkState(dirtyDirectDepIndex == 0, "Unexpected evaluation: %s", this);
     dirtyState = DirtyState.NEEDS_REBUILDING;
   }
 
   final void forceChanged() {
-    Preconditions.checkState(isDirty(), this);
-    Preconditions.checkState(!isChanged(), this);
-    Preconditions.checkState(isEvaluating(), this);
+    Preconditions.checkState(dirtyState == DirtyState.CHECK_DEPENDENCIES, this);
     Preconditions.checkState(getNumOfGroupsInLastBuildDirectDeps() == dirtyDirectDepIndex, this);
     dirtyState = DirtyState.REBUILDING;
   }
 
-  final int getSignaledDeps() {
-    return signaledDeps;
-  }
-
-  @Override
-  final boolean isDirty() {
-    return true;
-  }
-
-  @Override
   final boolean isChanged() {
     return dirtyState == DirtyState.NEEDS_REBUILDING || dirtyState == DirtyState.REBUILDING;
   }
 
-  @Override
-  protected final void checkFinishedBuildingWhenAboutToSetValue() {
-    Preconditions.checkState(isEvaluating(), "not started building %s", this);
+  private void checkFinishedBuildingWhenAboutToSetValue() {
     Preconditions.checkState(
-        !isDirty()
-            || dirtyState == DirtyState.VERIFIED_CLEAN
-            || dirtyState == DirtyState.REBUILDING,
+        dirtyState == DirtyState.VERIFIED_CLEAN || dirtyState == DirtyState.REBUILDING,
         "not done building %s",
         this);
   }
@@ -117,19 +101,22 @@ public abstract class DirtyBuildingState extends BuildingState {
   /**
    * 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 {@link
-   * #isReady} and comparing {@link #dirtyDirectDepIndex} and {@link
+   * 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()}.
    */
-  @Override
-  final void signalDepInternal(boolean childChanged, int numDirectDeps) {
+  final void signalDepInternal(boolean childChanged, boolean isReady) {
+    Preconditions.checkState(
+        isChanged() || (dirtyState == DirtyState.CHECK_DEPENDENCIES && dirtyDirectDepIndex > 0),
+        "Unexpected not evaluating: %s",
+        this);
     if (!isChanged()) {
       // Synchronization isn't needed here because the only caller is NodeEntry, which does it
       // through the synchronized method signalDep(Version).
       if (childChanged) {
         dirtyState = DirtyState.NEEDS_REBUILDING;
       } else if (dirtyState == DirtyState.CHECK_DEPENDENCIES
-          && isReady(numDirectDeps)
+          && isReady
           && 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.
@@ -164,16 +151,8 @@ public abstract class DirtyBuildingState extends BuildingState {
     return getNumOfGroupsInLastBuildDirectDeps() == 0;
   }
 
-  /**
-   * Gets the current state of checking this dirty entry to see if it must be re-evaluated. Must be
-   * called each time evaluation of a dirty entry starts to find the proper action to perform next,
-   * as enumerated by {@link DirtyState}.
-   *
-   * @see NodeEntry#getDirtyState()
-   */
+  /** @see NodeEntry#getDirtyState() */
   final DirtyState getDirtyState() {
-    // Entry may not be ready if being built just for its errors.
-    Preconditions.checkState(isEvaluating(), "must be evaluating to get dirty state %s", this);
     return dirtyState;
   }
 
@@ -183,9 +162,7 @@ public abstract class DirtyBuildingState extends BuildingState {
    * <p>See {@link NodeEntry#getNextDirtyDirectDeps}.
    */
   final Collection<SkyKey> getNextDirtyDirectDeps() throws InterruptedException {
-    Preconditions.checkState(isDirty(), this);
     Preconditions.checkState(dirtyState == DirtyState.CHECK_DEPENDENCIES, this);
-    Preconditions.checkState(isEvaluating(), this);
     Preconditions.checkState(dirtyDirectDepIndex < getNumOfGroupsInLastBuildDirectDeps(), this);
     return getLastBuildDirectDeps().get(dirtyDirectDepIndex++);
   }
@@ -196,7 +173,6 @@ public abstract class DirtyBuildingState extends BuildingState {
    * process the returned set, and so subsequent calls to this method will return the empty set.
    */
   Set<SkyKey> getAllRemainingDirtyDirectDeps(boolean preservePosition) throws InterruptedException {
-    Preconditions.checkState(isDirty(), this);
     ImmutableSet.Builder<SkyKey> result = ImmutableSet.builder();
 
     for (int ind = dirtyDirectDepIndex; ind < getNumOfGroupsInLastBuildDirectDeps(); ind++) {
@@ -213,13 +189,17 @@ public abstract class DirtyBuildingState extends BuildingState {
     dirtyState = DirtyState.REBUILDING;
   }
 
-  @Override
   protected MoreObjects.ToStringHelper getStringHelper() {
-    return super.getStringHelper()
+    return MoreObjects.toStringHelper(this)
         .add("dirtyState", dirtyState)
         .add("dirtyDirectDepIndex", dirtyDirectDepIndex);
   }
 
+  @Override
+  public String toString() {
+    return getStringHelper().toString();
+  }
+
   private static class FullDirtyBuildingState extends DirtyBuildingState {
     private final GroupedList<SkyKey> lastBuildDirectDeps;
     private final SkyValue lastBuildValue;
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 7f8330195b..7ad9025d0c 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -39,12 +39,33 @@ import javax.annotation.Nullable;
  * say b depends on a. If a completes first, it's done. If it completes second, it needs to signal
  * b, and potentially re-schedule it. If b completes first, it must exit, because it will be
  * signaled (and re-scheduled) by a. If it completes second, it must signal (and re-schedule)
- * itself. However, if the Evaluator supported re-entrancy for a node, then this wouldn't have to
- * be so strict, because duplicate scheduling would be less problematic.
+ * itself. However, if the Evaluator supported re-entrancy for a node, then this wouldn't have to be
+ * so strict, because duplicate scheduling would be less problematic.
  *
- * <p>The transient state of an {@code InMemoryNodeEntry} is kept in a {@link BuildingState} object.
- * Many of the methods of {@code InMemoryNodeEntry} are just wrappers around the corresponding
- * {@link BuildingState} methods.
+ * <p>During its life, a node can go through states as follows:
+ *
+ * <ol>
+ *   <li>Non-existent
+ *   <li>Just created ({@link #isEvaluating} is false)
+ *   <li>Evaluating ({@link #isEvaluating} is true)
+ *   <li>Done ({@link #isDone} is true)
+ *   <li>Just created (when it is dirtied: {@link #dirtyBuildingState} is not null)
+ *   <li>Reset (just before it is re-evaluated: {@link #dirtyBuildingState#getDirtyState} returns
+ *       {@link DirtyState#NEEDS_REBUILDING})
+ *   <li>Evaluating
+ *   <li>Done
+ * </ol>
+ *
+ * <p>The "just created" state is there to allow the {@link EvaluableGraph#createIfAbsentBatch} and
+ * {@link NodeEntry#addReverseDepAndCheckIfDone} methods to be separate. All callers have to call
+ * both methods in that order if they want to create a node. The second method transitions the
+ * current node to the "evaluating" state and returns true only the first time it was called. A
+ * caller that gets "true" back from that call must start the evaluation of this node, while any
+ * subsequent callers must not.
+ *
+ * <p>An entry is set to "evaluating" as soon as it is scheduled for evaluation. Thus, even a node
+ * that is never actually built (for instance, a dirty node that is verified as clean) is in the
+ * "evaluating" state until it is done.
  *
  * <p>This class is public only for the benefit of alternative graph implementations outside of the
  * package.
@@ -117,10 +138,35 @@ public class InMemoryNodeEntry implements NodeEntry {
   private List<Object> reverseDepsDataToConsolidate = null;
 
   /**
-   * The transient state of this entry, after it has been created but before it is done. It allows
-   * us to keep the current state of the entry across invalidation and successive evaluations.
+   * Object encapsulating dirty state of the object between when it is marked dirty and
+   * re-evaluated.
+   */
+  @VisibleForTesting @Nullable protected volatile DirtyBuildingState dirtyBuildingState = null;
+
+  private static final int NOT_EVALUATING_SENTINEL = -1;
+
+  /**
+   * The number of dependencies that are known to be done in a {@link NodeEntry} if it is already
+   * evaluating, and a sentinel (-1) indicating that it has not yet started evaluating otherwise.
+   * 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>The first problem is solved by the {@link #signalDep} method, which 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
+   * #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
+   * #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
+   * {@code ParallelEvaluator.Evaluate#run}.
    */
-  @VisibleForTesting @Nullable protected volatile BuildingState buildingState = new BuildingState();
+  private int signaledDeps = NOT_EVALUATING_SENTINEL;
 
   /**
    * Construct a InMemoryNodeEntry. Use ONLY in Skyframe evaluation and graph implementations.
@@ -135,7 +181,7 @@ public class InMemoryNodeEntry implements NodeEntry {
 
   @Override
   public boolean isDone() {
-    return buildingState == null;
+    return value != null && !isEvaluating();
   }
 
   @Override
@@ -191,7 +237,7 @@ public class InMemoryNodeEntry implements NodeEntry {
   }
 
   private DirtyBuildingState getDirtyBuildingState() {
-    return (DirtyBuildingState) Preconditions.checkNotNull(buildingState, this);
+    return Preconditions.checkNotNull(dirtyBuildingState, "Didn't have state: %s", this);
   }
 
   /**
@@ -199,7 +245,8 @@ public class InMemoryNodeEntry implements NodeEntry {
    * need to override the other.
    */
   protected void markDone() {
-    buildingState = null;
+    dirtyBuildingState = null;
+    signaledDeps = NOT_EVALUATING_SENTINEL;
   }
 
   protected synchronized Set<SkyKey> setStateFinishedAndReturnReverseDepsToSignal() {
@@ -261,8 +308,11 @@ public class InMemoryNodeEntry implements NodeEntry {
     if (isDone()) {
       return DependencyState.DONE;
     }
-    return buildingState.startEvaluating() ? DependencyState.NEEDS_SCHEDULING
-                                           : DependencyState.ALREADY_EVALUATING;
+    boolean result = !isEvaluating();
+    if (result) {
+      signaledDeps = 0;
+    }
+    return result ? DependencyState.NEEDS_SCHEDULING : DependencyState.ALREADY_EVALUATING;
   }
 
   /** Sets {@link #reverseDeps}. Does not alter {@link #reverseDepsDataToConsolidate}. */
@@ -360,19 +410,22 @@ public class InMemoryNodeEntry implements NodeEntry {
   @Override
   public synchronized boolean signalDep(Version childVersion) {
     Preconditions.checkState(!isDone(), "Value must not be done in signalDep %s", this);
-    return buildingState.signalDep(
-        /*childChanged=*/ !childVersion.atMost(lastEvaluatedVersion),
-        getTemporaryDirectDeps().numElements());
+    Preconditions.checkState(isEvaluating(), this);
+    signaledDeps++;
+    if (isDirty()) {
+      dirtyBuildingState.signalDepInternal(!childVersion.atMost(lastEvaluatedVersion), isReady());
+    }
+    return isReady();
   }
 
   @Override
   public synchronized boolean isDirty() {
-    return !isDone() && buildingState.isDirty();
+    return !isDone() && dirtyBuildingState != null;
   }
 
   @Override
   public synchronized boolean isChanged() {
-    return !isDone() && buildingState.isChanged();
+    return !isDone() && dirtyBuildingState != null && dirtyBuildingState.isChanged();
   }
 
   /** Checks that a caller is not trying to access not-stored graph edges. */
@@ -384,7 +437,7 @@ public class InMemoryNodeEntry implements NodeEntry {
   public synchronized MarkedDirtyResult markDirty(boolean isChanged) {
     assertKeepEdges();
     if (isDone()) {
-      buildingState =
+      dirtyBuildingState =
           DirtyBuildingState.create(isChanged, GroupedList.<SkyKey>create(directDeps), value);
       value = null;
       directDeps = null;
@@ -414,14 +467,13 @@ public class InMemoryNodeEntry implements NodeEntry {
         "Direct deps must be the same as those found last build for node to be marked clean: %s",
         this);
     Preconditions.checkState(isDirty(), this);
-    Preconditions.checkState(!buildingState.isChanged(), "shouldn't be changed: %s", this);
+    Preconditions.checkState(!dirtyBuildingState.isChanged(), "shouldn't be changed: %s", this);
     return setStateFinishedAndReturnReverseDepsToSignal();
   }
 
   @Override
   public synchronized void forceRebuild() {
-    Preconditions.checkState(
-        getTemporaryDirectDeps().numElements() == getDirtyBuildingState().getSignaledDeps(), this);
+    Preconditions.checkState(getTemporaryDirectDeps().numElements() == signaledDeps, this);
     getDirtyBuildingState().forceChanged();
   }
 
@@ -430,9 +482,9 @@ public class InMemoryNodeEntry implements NodeEntry {
     return lastChangedVersion;
   }
 
-  /** @see DirtyBuildingState#getDirtyState() */
   @Override
   public synchronized NodeEntry.DirtyState getDirtyState() {
+    Preconditions.checkState(isEvaluating(), "Not evaluating for dirty state? %s", this);
     return getDirtyBuildingState().getDirtyState();
   }
 
@@ -440,6 +492,7 @@ public class InMemoryNodeEntry implements NodeEntry {
   @Override
   public synchronized Collection<SkyKey> getNextDirtyDirectDeps() throws InterruptedException {
     Preconditions.checkState(isReady(), this);
+    Preconditions.checkState(isEvaluating(), "Not evaluating during getNextDirty? %s", this);
     return getDirtyBuildingState().getNextDirtyDirectDeps();
   }
 
@@ -463,12 +516,12 @@ public class InMemoryNodeEntry implements NodeEntry {
 
   @Override
   public synchronized Set<SkyKey> getAllRemainingDirtyDirectDeps() throws InterruptedException {
+    Preconditions.checkState(isEvaluating(), "Not evaluating for remaining dirty? %s", this);
     if (isDirty()) {
       Preconditions.checkState(
           getDirtyBuildingState().getDirtyState() == DirtyState.REBUILDING, this);
       return getDirtyBuildingState().getAllRemainingDirtyDirectDeps(/*preservePosition=*/ true);
     } else {
-      Preconditions.checkState(buildingState.isEvaluating(), this);
       return ImmutableSet.of();
     }
   }
@@ -521,7 +574,17 @@ public class InMemoryNodeEntry implements NodeEntry {
   @Override
   public synchronized boolean isReady() {
     Preconditions.checkState(!isDone(), "can't be ready if done: %s", this);
-    return buildingState.isReady(getTemporaryDirectDeps().numElements());
+    return isReady(getTemporaryDirectDeps().numElements());
+  }
+
+  /** Returns whether all known children of this node have signaled that they are done. */
+  private boolean isReady(int numDirectDeps) {
+    Preconditions.checkState(signaledDeps <= numDirectDeps, "%s %s", numDirectDeps, this);
+    return signaledDeps == numDirectDeps;
+  }
+
+  private boolean isEvaluating() {
+    return signaledDeps > NOT_EVALUATING_SENTINEL;
   }
 
   @Override
@@ -532,8 +595,9 @@ public class InMemoryNodeEntry implements NodeEntry {
         .add("lastChangedVersion", lastChangedVersion)
         .add("lastEvaluatedVersion", lastEvaluatedVersion)
         .add("directDeps", isDone() ? GroupedList.create(directDeps) : directDeps)
+        .add("signaledDeps", signaledDeps)
         .add("reverseDeps", ReverseDepsUtility.toString(this))
-        .add("buildingState", buildingState)
+        .add("dirtyBuildingState", dirtyBuildingState)
         .toString();
   }
 
@@ -551,7 +615,7 @@ public class InMemoryNodeEntry implements NodeEntry {
     nodeEntry.lastEvaluatedVersion = this.lastEvaluatedVersion;
     ReverseDepsUtility.addReverseDeps(nodeEntry, ReverseDepsUtility.getReverseDeps(this));
     nodeEntry.directDeps = directDeps;
-    nodeEntry.buildingState = null;
+    nodeEntry.dirtyBuildingState = null;
     return nodeEntry;
   }
 }