Skip to content

Commit

Permalink
Handle resetForRestartFromScratch on an incremental build.
Browse files Browse the repository at this point in the history
Similar to b4926ef, a special `DirtyBuildingState` implementation is used to keep track of deps registered prior to a reset. On an incremental build, however, there is even more state to keep track of.

PiperOrigin-RevId: 561651160
Change-Id: I9b6156af0abccd1e56558be74f2b1cbb1c8641b8
  • Loading branch information
justinhorvitz authored and copybara-github committed Aug 31, 2023
1 parent f1131dd commit 084a876
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ public synchronized boolean signalDep(Version childVersion, @Nullable SkyKey chi
@ForOverride
protected DirtyBuildingState createDirtyBuildingStateForDoneNode(
DirtyType dirtyType, GroupedDeps directDeps, SkyValue value) {
return new IncrementalDirtyBuildingState(dirtyType, directDeps, value);
return new IncrementalBuildingState(dirtyType, directDeps, value);
}

@Nullable
Expand Down Expand Up @@ -391,16 +391,23 @@ final synchronized int getNumTemporaryDirectDeps() {
public final synchronized void resetForRestartFromScratch() {
checkState(!hasUnsignaledDeps(), this);

// TODO(b/228090759): Handle a reset on an incremental build.
checkState(!dirtyBuildingState.isIncremental(), this);

ImmutableSet<SkyKey> resetDeps =
ImmutableSet.<SkyKey>builder()
.addAll(getResetDirectDeps()) // In case this isn't the first reset.
.addAll(getTemporaryDirectDeps().getAllElementsAsIterable())
.build();

dirtyBuildingState = new ResetInitialBuildingState(resetDeps);
if (dirtyBuildingState.isIncremental()) {
var incrementalBuildingState = (IncrementalBuildingState) dirtyBuildingState;
dirtyBuildingState =
new ResetIncrementalBuildingState(
incrementalBuildingState.lastBuildDirectDeps,
incrementalBuildingState.lastBuildValue,
incrementalBuildingState.dirtyDirectDepIndex,
resetDeps);
} else {
dirtyBuildingState = new ResetInitialBuildingState(resetDeps);
}
directDeps = null;
}

Expand All @@ -420,34 +427,34 @@ protected synchronized MoreObjects.ToStringHelper toStringHelper() {
}

/** {@link DirtyBuildingState} for a node on an incremental build. */
private static final class IncrementalDirtyBuildingState extends DirtyBuildingState {
private static class IncrementalBuildingState extends DirtyBuildingState {
private final GroupedDeps lastBuildDirectDeps;
private final SkyValue lastBuildValue;

private IncrementalDirtyBuildingState(
private IncrementalBuildingState(
DirtyType dirtyType, GroupedDeps lastBuildDirectDeps, SkyValue lastBuildValue) {
super(dirtyType);
this.lastBuildDirectDeps = lastBuildDirectDeps;
this.lastBuildValue = lastBuildValue;
}

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

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

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

@Override
protected int getNumOfGroupsInLastBuildDirectDeps() {
protected final int getNumOfGroupsInLastBuildDirectDeps() {
return lastBuildDirectDeps.numGroups();
}

Expand Down Expand Up @@ -482,4 +489,35 @@ protected MoreObjects.ToStringHelper getStringHelper() {
return super.getStringHelper().add("resetDeps", resetDeps);
}
}

/**
* Used to track already registered deps when there is a {@linkplain #resetForRestartFromScratch
* reset} on a node's incremental build.
*/
private static final class ResetIncrementalBuildingState extends IncrementalBuildingState {
private final ImmutableSet<SkyKey> resetDeps;

private ResetIncrementalBuildingState(
GroupedDeps lastBuildDirectDeps,
SkyValue lastBuildValue,
int dirtyDirectDepIndex,
ImmutableSet<SkyKey> resetDeps) {
// CHANGE (not DIRTY) since we already know it needs rebuilding.
super(DirtyType.CHANGE, lastBuildDirectDeps, lastBuildValue);
this.dirtyDirectDepIndex = dirtyDirectDepIndex;
this.resetDeps = resetDeps;
markRebuilding();
startEvaluating();
}

@Override
ImmutableSet<SkyKey> getResetDirectDeps() {
return resetDeps;
}

@Override
protected MoreObjects.ToStringHelper getStringHelper() {
return super.getStringHelper().add("resetDeps", resetDeps);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.devtools.build.skyframe.NodeEntry.DirtyType;
import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.testing.junit.testparameterinjector.TestParameter;
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.util.ArrayList;
import java.util.List;
Expand All @@ -35,7 +36,7 @@
@RunWith(TestParameterInjector.class)
public class IncrementalInMemoryNodeEntryTest extends InMemoryNodeEntryTest<IntVersion> {

protected final Version incrementalVersion = initialVersion.next();
protected final IntVersion incrementalVersion = initialVersion.next();

@Override
protected IncrementalInMemoryNodeEntry createEntry(SkyKey key) {
Expand Down Expand Up @@ -629,4 +630,83 @@ public void hasAtLeastOneDep_false() throws Exception {
entry.setValue(new IntegerValue(1), initialVersion, null);
assertThat(entry.hasAtLeastOneDep()).isFalse();
}

@Test
public void resetOnDirtyNode(@TestParameter boolean valueChanges) throws Exception {
InMemoryNodeEntry entry = createEntry();
entry.addReverseDepAndCheckIfDone(null); // Start evaluation.
entry.markRebuilding();

// Rdep added on initial build that stays on incremental build.
SkyKey oldAndNewParent = key("oldAndNewParent");
entry.addReverseDepAndCheckIfDone(oldAndNewParent);

// Rdep added on initial build that is removed on incremental build.
SkyKey oldParent = key("oldParent");
entry.addReverseDepAndCheckIfDone(oldParent);

// Dep added on initial build that stays on incremental build.
SkyKey oldAndNewDep = key("oldAndNewDep");
entry.addSingletonTemporaryDirectDep(oldAndNewDep);
entry.signalDep(initialVersion, oldAndNewDep);

// Dep added on initial build that is removed on incremental build.
SkyKey oldDep = key("oldDep");
entry.addSingletonTemporaryDirectDep(oldDep);
entry.signalDep(initialVersion, oldDep);

// Initial build completes.
SkyValue oldValue = new IntegerValue(1);
setValue(entry, oldValue, /* errorInfo= */ null, initialVersion);

// Start of incremental build.
entry.markDirty(DirtyType.DIRTY);

// One rdep added, one rdep stays, one rdep removed.
SkyKey newParent = key("newParent");
entry.addReverseDepAndCheckIfDone(newParent);
entry.checkIfDoneForDirtyReverseDep(oldAndNewParent);
entry.removeReverseDep(oldParent);

// Old dep added before reset, triggers rebuild.
assertThat(entry.getNextDirtyDirectDeps()).containsExactly(oldAndNewDep);
entry.addSingletonTemporaryDirectDep(oldAndNewDep);
entry.signalDep(incrementalVersion, oldAndNewDep);
entry.markRebuilding();
assertThat(entry.getResetDirectDeps()).isEmpty();

// Reset clears temporary direct deps.
entry.resetForRestartFromScratch();
assertThat(entry.getDirtyState()).isEqualTo(DirtyState.REBUILDING);
assertThat(entry.getTemporaryDirectDeps()).isEmpty();
assertThat(entry.getTemporaryDirectDeps() instanceof GroupedDeps.WithHashSet)
.isEqualTo(isPartialReevaluation);

// Add back same dep.
entry.addSingletonTemporaryDirectDep(oldAndNewDep);
assertThat(entry.signalDep(incrementalVersion, oldAndNewDep)).isTrue();
assertThat(entry.getTemporaryDirectDeps()).containsExactly(ImmutableList.of(oldAndNewDep));

// New dep added after reset.
SkyKey newDep = key("newDep");
entry.addSingletonTemporaryDirectDep(newDep);
assertThat(entry.signalDep(incrementalVersion, newDep)).isTrue();
assertThat(entry.getTemporaryDirectDeps())
.containsExactly(ImmutableList.of(oldAndNewDep), ImmutableList.of(newDep));

// Check dep accounting.
assertThat(entry.getResetDirectDeps()).containsExactly(oldAndNewDep);
assertThat(entry.getAllRemainingDirtyDirectDeps()).containsExactly(oldDep);

// Set value and check that new parents will be signaled.
SkyValue newValue = valueChanges ? new IntegerValue(2) : oldValue;
assertThat(setValue(entry, newValue, /* errorInfo= */ null, incrementalVersion))
.containsExactly(oldAndNewParent, newParent);

if (valueChanges) {
assertThat(entry.getVersion()).isEqualTo(incrementalVersion);
} else {
assertThat(entry.getVersion()).isEqualTo(initialVersion); // Change pruning.
}
}
}

0 comments on commit 084a876

Please sign in to comment.