Skip to content

Commit

Permalink
Implement NonIncrementalInMemoryNodeEntry separately from `Incremen…
Browse files Browse the repository at this point in the history 
…talInMemoryNodeEntry`.

This reduces the shallow heap of `NonIncrementalInMemoryNodeEntry` from 40 to 24 bytes, which is especially good because users often opt for non-incremental builds to save memory. The overall savings are somewhat lessened by existing optimizations which already reduce the number of retained nodes in non-incremental builds:

* `--heuristically_drop_nodes` which discards nodes on the fly.
* `GlobbingStrategy#NON_SKYFRAME`, which is used to reduce the number of nodes in the glob subgraph on non-incremental builds since we don't need incremental correctness.

Some of the state checks for the "force rebuild" lifecycle are loosened to allow `NonIncrementalInMemoryNodeEntry` to act as if it has never been built before, as opposed to supporting some special incremental building state only for rewinding. I left a couple `TODO`s to tighten this up as I work on incremental rewinding support.

PiperOrigin-RevId: 555934542
Change-Id: Ie9600706c7f34b41a11401812534571e516e5a3b
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Aug 11, 2023
1 parent 7bcd5cc commit 20c541c
Show file tree
Hide file tree
Showing 8 changed files with 324 additions and 92 deletions.
8 changes: 6 additions & 2 deletions src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryNodeEntry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,12 @@
* the ALREADY_EVALUATING state until it is DONE.
*
* <p>From the DONE state, the node can go back to the "marked as affected" state.
*
* @param <D> the type of {@link DirtyBuildingState} used by the {@link AbstractInMemoryNodeEntry}
* subclass
*/
abstract class AbstractInMemoryNodeEntry implements InMemoryNodeEntry {
abstract class AbstractInMemoryNodeEntry<D extends DirtyBuildingState>
implements InMemoryNodeEntry {

private final SkyKey key;

Expand All @@ -70,7 +74,7 @@ abstract class AbstractInMemoryNodeEntry implements InMemoryNodeEntry {
* Tracks state of this entry while it is evaluating (either on its initial build or after being
* marked dirty).
*/
@Nullable protected volatile DirtyBuildingState dirtyBuildingState;
@Nullable protected volatile D dirtyBuildingState;

AbstractInMemoryNodeEntry(SkyKey key) {
this.key = checkNotNull(key);
Expand Down
26 changes: 17 additions & 9 deletions src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
* <p>If the node has previously been built, {@link #isIncremental} returns true. Deps are checked
* to see if re-evaluation is needed, and the node will either marked clean or re-evaluated.
*
* <p>This class does not attempt to synchronize operations. It is assumed that the calling {@link
* InMemoryNodeEntry} performs the appropriate synchronization when necessary.
*
* <p>This class is public only for the benefit of alternative graph implementations outside of the
* package.
*/
Expand Down Expand Up @@ -165,14 +168,21 @@ final void markForceRebuild() {
}
}

// TODO(b/228090759): Tighten up state checks for the force rebuild lifecycle.
final void forceRebuild(int numTemporaryDirectDeps) {
checkState(numTemporaryDirectDeps + externalDeps == signaledDeps, this);
checkState(
(dirtyState == DirtyState.CHECK_DEPENDENCIES
&& getNumOfGroupsInLastBuildDirectDeps() == dirtyDirectDepIndex)
|| dirtyState == DirtyState.NEEDS_FORCED_REBUILDING,
this);
dirtyState = DirtyState.REBUILDING;
switch (dirtyState) {
case CHECK_DEPENDENCIES:
checkState(getNumOfGroupsInLastBuildDirectDeps() == dirtyDirectDepIndex, this);
dirtyState = DirtyState.REBUILDING;
break;
case NEEDS_REBUILDING: // Valid for NonIncrementalInMemoryNodeEntry.
case NEEDS_FORCED_REBUILDING: // Valid for IncrementalInMemoryNodeEntry.
dirtyState = DirtyState.REBUILDING;
break;
default:
throw new IllegalStateException("Unexpected dirty state " + dirtyState + ": " + this);
}
}

final boolean isEvaluating() {
Expand Down Expand Up @@ -202,12 +212,10 @@ private void checkFinishedBuildingWhenAboutToSetValue() {
* DirtyBuildingState#getNumOfGroupsInLastBuildDirectDeps()}.
*/
final void signalDep(
AbstractInMemoryNodeEntry entry,
AbstractInMemoryNodeEntry<?> entry,
NodeVersion version,
Version childVersion,
@Nullable SkyKey childForDebugging) {
// Synchronization isn't needed here because the only caller is InMemoryNodeEntry, which does it
// through the synchronized method signalDep.
checkState(isEvaluating(), "%s %s", entry, childForDebugging);
signaledDeps++;
if (isChanged()) {
Expand Down
59 changes: 14 additions & 45 deletions src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.skyframe;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;

Expand All @@ -29,7 +28,7 @@
import javax.annotation.Nullable;

/** An {@link InMemoryNodeEntry} that {@link #keepsEdges} for use in incremental evaluations. */
public class IncrementalInMemoryNodeEntry extends AbstractInMemoryNodeEntry {
public class IncrementalInMemoryNodeEntry extends AbstractInMemoryNodeEntry<DirtyBuildingState> {

protected volatile NodeVersion version = Version.minimal();

Expand All @@ -38,8 +37,8 @@ public class IncrementalInMemoryNodeEntry extends AbstractInMemoryNodeEntry {
* requested them that way. It contains either the in-progress direct deps, stored as a {@link
* GroupedDeps} (constructed via {@link GroupedDeps.WithHashSet} if {@code
* key.supportsPartialReevaluation()}) before the node is finished building, or the full direct
* deps, compressed in a memory-efficient way (via {@link GroupedDeps#compress}, after the node is
* done.
* deps, compressed in a memory-efficient way (via {@link GroupedDeps#compress}), after the node
* is done.
*
* <p>It is initialized lazily in getTemporaryDirectDeps() to save a little memory.
*/
Expand Down Expand Up @@ -84,7 +83,7 @@ public IncrementalInMemoryNodeEntry(SkyKey key) {
}

@Override
public boolean keepsEdges() {
public final boolean keepsEdges() {
return true;
}

Expand Down Expand Up @@ -123,7 +122,6 @@ public boolean hasAtLeastOneDep() {

@Override
public final synchronized @GroupedDeps.Compressed Object getCompressedDirectDepsForDoneEntry() {
assertKeepEdges();
checkState(isDone(), "no deps until done. NodeEntry: %s", this);
checkNotNull(directDeps, "deps can't be null: %s", this);
return GroupedDeps.castAsCompressed(directDeps);
Expand All @@ -140,7 +138,7 @@ protected void markDone() {

protected final synchronized Set<SkyKey> setStateFinishedAndReturnReverseDepsToSignal() {
Set<SkyKey> reverseDepsToSignal = ReverseDepsUtility.consolidateDataAndReturnNewElements(this);
directDeps = keepsEdges() ? getTemporaryDirectDeps().compress() : null;
directDeps = getTemporaryDirectDeps().compress();
markDone();
return reverseDepsToSignal;
}
Expand All @@ -162,10 +160,6 @@ public synchronized Set<SkyKey> getInProgressReverseDeps() {
public synchronized Set<SkyKey> setValue(
SkyValue value, Version graphVersion, @Nullable Version maxTransitiveSourceVersion)
throws InterruptedException {
checkArgument(
keepsEdges() || graphVersion.equals(ConstantVersion.INSTANCE),
"Non-incremental evaluations must use a constant graph version, got %s",
graphVersion);
checkState(!hasUnsignaledDeps(), "Has unsignaled deps (this=%s, value=%s)", this, value);
checkState(
version.lastChanged().atMost(graphVersion) && version.lastEvaluated().atMost(graphVersion),
Expand All @@ -192,7 +186,7 @@ public synchronized Set<SkyKey> setValue(
@Override
@CanIgnoreReturnValue
public DependencyState addReverseDepAndCheckIfDone(SkyKey reverseDep) {
if ((reverseDep == null || !keepsEdges()) && isDone()) {
if (reverseDep == null && isDone()) {
return DependencyState.DONE;
}

Expand All @@ -203,21 +197,19 @@ public DependencyState addReverseDepAndCheckIfDone(SkyKey reverseDep) {
}
if (reverseDep != null) {
if (done) {
if (keepsEdges()) {
ReverseDepsUtility.addReverseDep(this, reverseDep);
}
ReverseDepsUtility.addReverseDep(this, reverseDep);
} else {
appendToReverseDepOperations(reverseDep, Op.ADD);
}
}
if (done) {
return DependencyState.DONE;
}
boolean wasEvaluating = dirtyBuildingState.isEvaluating();
if (!wasEvaluating) {
dirtyBuildingState.startEvaluating();
if (dirtyBuildingState.isEvaluating()) {
return DependencyState.ALREADY_EVALUATING;
}
return wasEvaluating ? DependencyState.ALREADY_EVALUATING : DependencyState.NEEDS_SCHEDULING;
dirtyBuildingState.startEvaluating();
return DependencyState.NEEDS_SCHEDULING;
}
}

Expand Down Expand Up @@ -252,7 +244,6 @@ private synchronized void appendToReverseDepOperations(SkyKey reverseDep, Op op)
@Override
public synchronized DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverseDep) {
checkNotNull(reverseDep, this);
checkState(keepsEdges(), "Incremental means keeping edges %s %s", reverseDep, this);
if (isDone()) {
return DependencyState.DONE;
}
Expand All @@ -262,9 +253,6 @@ public synchronized DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverse

@Override
public synchronized void removeReverseDep(SkyKey reverseDep) {
if (!keepsEdges()) {
return;
}
if (isDone()) {
ReverseDepsUtility.removeReverseDep(this, reverseDep);
} else {
Expand All @@ -276,7 +264,6 @@ public synchronized void removeReverseDep(SkyKey reverseDep) {

@Override
public synchronized void removeReverseDepsFromDoneEntryDueToDeletion(Set<SkyKey> deletedKeys) {
assertKeepEdges();
checkState(isDone(), this);
ReverseDepsUtility.removeReverseDepsMatching(this, deletedKeys);
}
Expand All @@ -288,14 +275,12 @@ public synchronized void removeInProgressReverseDep(SkyKey reverseDep) {

@Override
public synchronized Collection<SkyKey> getReverseDepsForDoneEntry() {
assertKeepEdges();
checkState(isDone(), "Called on not done %s", this);
return ReverseDepsUtility.getReverseDeps(this, /* checkConsistency= */ true);
}

@Override
public synchronized Collection<SkyKey> getAllReverseDepsForNodeBeingDeleted() {
assertKeepEdges();
if (!isDone()) {
// This consolidation loses information about pending reverse deps to signal, but that is
// unimportant since this node is being deleted.
Expand All @@ -313,11 +298,6 @@ public synchronized boolean signalDep(Version childVersion, @Nullable SkyKey chi
return !hasUnsignaledDeps();
}

/** Checks that a caller is not trying to access not-stored graph edges. */
private void assertKeepEdges() {
checkState(keepsEdges(), "Not keeping edges: %s", this);
}

/**
* Creates a {@link DirtyBuildingState} for the case where this node is done and is being marked
* dirty.
Expand All @@ -328,25 +308,16 @@ protected DirtyBuildingState createDirtyBuildingStateForDoneNode(
return new IncrementalDirtyBuildingState(dirtyType, getKey(), directDeps, value);
}

private static final GroupedDeps EMPTY_LIST = new GroupedDeps();

@Nullable
@Override
public synchronized MarkedDirtyResult markDirty(DirtyType dirtyType) {
if (!DirtyType.FORCE_REBUILD.equals(dirtyType)) {
// A node can't be found to be dirty without deps unless it's force-rebuilt.
assertKeepEdges();
}
if (isDone()) {
GroupedDeps directDeps =
keepsEdges() ? GroupedDeps.decompress(getCompressedDirectDepsForDoneEntry()) : EMPTY_LIST;
GroupedDeps directDeps = GroupedDeps.decompress(getCompressedDirectDepsForDoneEntry());
dirtyBuildingState = createDirtyBuildingStateForDoneNode(dirtyType, directDeps, value);
value = null;
this.directDeps = null;
return MarkedDirtyResult.withReverseDeps(
keepsEdges()
? ReverseDepsUtility.getReverseDeps(this, /* checkConsistency= */ true)
: ImmutableList.of());
ReverseDepsUtility.getReverseDeps(this, /* checkConsistency= */ true));
}
if (dirtyType.equals(DirtyType.FORCE_REBUILD)) {
if (dirtyBuildingState != null) {
Expand Down Expand Up @@ -420,9 +391,7 @@ protected synchronized MoreObjects.ToStringHelper toStringHelper() {
.add("version", version)
.add(
"directDeps",
isDone() && keepsEdges()
? GroupedDeps.decompress(getCompressedDirectDepsForDoneEntry())
: directDeps)
isDone() ? GroupedDeps.decompress(getCompressedDirectDepsForDoneEntry()) : directDeps)
.add("reverseDeps", ReverseDepsUtility.toString(this));
}

Expand Down
15 changes: 9 additions & 6 deletions src/main/java/com/google/devtools/build/skyframe/InitialBuildingState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,32 +16,35 @@
import com.google.devtools.build.skyframe.NodeEntry.DirtyType;
import javax.annotation.Nullable;

/** {@link DirtyBuildingState} for a node on its initial build. */
final class InitialBuildingState extends DirtyBuildingState {
/**
* {@link DirtyBuildingState} for a node on its initial build or a {@link
* NonIncrementalInMemoryNodeEntry} being {@linkplain NodeEntry#forceRebuild force rebuilt}.
*/
class InitialBuildingState extends DirtyBuildingState {

InitialBuildingState(boolean hasLowFanout) {
super(DirtyType.CHANGE, hasLowFanout);
}

@Nullable
@Override
public GroupedDeps getLastBuildDirectDeps() {
public final GroupedDeps getLastBuildDirectDeps() {
return null;
}

@Override
protected int getNumOfGroupsInLastBuildDirectDeps() {
protected final int getNumOfGroupsInLastBuildDirectDeps() {
return 0;
}

@Nullable
@Override
public SkyValue getLastBuildValue() {
public final SkyValue getLastBuildValue() {
return null;
}

@Override
protected boolean isIncremental() {
protected final boolean isIncremental() {
return false;
}
}
Loading

0 comments on commit 20c541c

Please sign in to comment.