Skip to content

Commit

Permalink
Require non-incremental evaluations to use a ConstantVersion.
Browse files Browse the repository at this point in the history 
In practice, this was fulfilled in bazelbuild@87181b8. Adding the `checkArgument` ensures that tests are being realistic.

PiperOrigin-RevId: 555215331
Change-Id: I055b0610f1a629f9cbf779fd760f14d9c3ec29dd
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Aug 9, 2023
1 parent 1c8a771 commit 53c3518
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 103 deletions.
5 changes: 5 additions & 0 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,6 +13,7 @@
// 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 Down Expand Up @@ -161,6 +162,10 @@ 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 Down
5 changes: 5 additions & 0 deletions src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ protected ProcessableGraph getGraph(Version version) {
return new EdgelessInMemoryGraphImpl(/* usePooledInterning= */ true);
}

@Override
protected Version getStartingVersion() {
return ConstantVersion.INSTANCE;
}

@Override
protected Version getNextVersion(Version version) {
throw new UnsupportedOperationException();
Expand Down
56 changes: 29 additions & 27 deletions src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.skyframe;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.skyframe.NodeEntrySubjectFactory.assertThatNodeEntry;
import static org.junit.Assert.assertThrows;
Expand All @@ -28,6 +29,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.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.annotations.ForOverride;
import com.google.testing.junit.testparameterinjector.TestParameter;
import java.util.Set;
Expand Down Expand Up @@ -57,14 +59,13 @@ public boolean supportsPartialReevaluation() {
}
};

static final IntVersion ZERO_VERSION = IntVersion.of(0L);
static final IntVersion ONE_VERSION = IntVersion.of(1L);

private static final NestedSet<Reportable> NO_EVENTS =
NestedSetBuilder.emptySet(Order.STABLE_ORDER);

@TestParameter boolean isPartialReevaluation;

final Version initialVersion = keepEdges() ? IntVersion.of(0) : ConstantVersion.INSTANCE;

static SkyKey key(String name) {
return GraphTester.skyKey(name);
}
Expand Down Expand Up @@ -102,7 +103,7 @@ public void signalEntry() throws InterruptedException {
entry.addSingletonTemporaryDirectDep(dep1);
assertThat(entry.isReadyToEvaluate()).isEqualTo(isPartialReevaluation);
assertThat(entry.hasUnsignaledDeps()).isTrue();
assertThat(entry.signalDep(ZERO_VERSION, dep1)).isTrue();
assertThat(entry.signalDep(initialVersion, dep1)).isTrue();
assertThat(entry.isReadyToEvaluate()).isTrue();
assertThat(entry.hasUnsignaledDeps()).isFalse();
assertThatNodeEntry(entry).hasTemporaryDirectDepsThat().containsExactly(dep1);
Expand All @@ -112,16 +113,15 @@ public void signalEntry() throws InterruptedException {
entry.addSingletonTemporaryDirectDep(dep3);
assertThat(entry.isReadyToEvaluate()).isEqualTo(isPartialReevaluation);
assertThat(entry.hasUnsignaledDeps()).isTrue();
assertThat(entry.signalDep(ZERO_VERSION, dep2)).isFalse();
assertThat(entry.signalDep(initialVersion, dep2)).isFalse();
assertThat(entry.isReadyToEvaluate()).isEqualTo(isPartialReevaluation);
assertThat(entry.hasUnsignaledDeps()).isTrue();
assertThat(entry.signalDep(ZERO_VERSION, dep3)).isTrue();
assertThat(entry.signalDep(initialVersion, dep3)).isTrue();
assertThat(entry.isReadyToEvaluate()).isTrue();
assertThat(entry.hasUnsignaledDeps()).isFalse();
assertThat(setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 0L))
.isEmpty();
assertThat(setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion)).isEmpty();
assertThat(entry.isDone()).isTrue();
assertThat(entry.getVersion()).isEqualTo(ZERO_VERSION);
assertThat(entry.getVersion()).isEqualTo(initialVersion);

if (!keepEdges()) {
return;
Expand All @@ -138,13 +138,13 @@ public void signalExternalDep() throws InterruptedException {
entry.addExternalDep();
assertThat(entry.isReadyToEvaluate()).isEqualTo(isPartialReevaluation);
assertThat(entry.hasUnsignaledDeps()).isTrue();
assertThat(entry.signalDep(ZERO_VERSION, null)).isTrue();
assertThat(entry.signalDep(initialVersion, null)).isTrue();
assertThat(entry.isReadyToEvaluate()).isTrue();
assertThat(entry.hasUnsignaledDeps()).isFalse();
entry.addExternalDep();
assertThat(entry.isReadyToEvaluate()).isEqualTo(isPartialReevaluation);
assertThat(entry.hasUnsignaledDeps()).isTrue();
assertThat(entry.signalDep(ZERO_VERSION, null)).isTrue();
assertThat(entry.signalDep(initialVersion, null)).isTrue();
assertThat(entry.isReadyToEvaluate()).isTrue();
assertThat(entry.hasUnsignaledDeps()).isFalse();
assertThatNodeEntry(entry).hasTemporaryDirectDepsThat().containsExactly();
Expand All @@ -162,7 +162,7 @@ public void reverseDeps() throws InterruptedException {
assertThat(entry.addReverseDepAndCheckIfDone(father))
.isEqualTo(DependencyState.ALREADY_EVALUATING);
entry.markRebuilding();
assertThat(setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 0L))
assertThat(setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion))
.containsExactly(mother, father);

if (!keepEdges()) {
Expand All @@ -184,7 +184,7 @@ public void errorValue() throws InterruptedException {
new ReifiedSkyFunctionException(
new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT));
ErrorInfo errorInfo = ErrorInfo.fromException(exception, false);
assertThat(setValue(entry, /* value= */ null, errorInfo, /* graphVersion= */ 0L)).isEmpty();
assertThat(setValue(entry, /* value= */ null, errorInfo, initialVersion)).isEmpty();
assertThat(entry.isDone()).isTrue();
assertThat(entry.getValue()).isNull();
assertThat(entry.getErrorInfo()).isEqualTo(errorInfo);
Expand All @@ -199,7 +199,7 @@ public void errorAndValue() throws InterruptedException {
new ReifiedSkyFunctionException(
new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT));
ErrorInfo errorInfo = ErrorInfo.fromException(exception, false);
setValue(entry, new SkyValue() {}, errorInfo, /* graphVersion= */ 0L);
setValue(entry, new SkyValue() {}, errorInfo, initialVersion);
assertThat(entry.isDone()).isTrue();
assertThat(entry.getErrorInfo()).isEqualTo(errorInfo);
}
Expand All @@ -211,27 +211,27 @@ public void crashOnNullErrorAndValue() throws InterruptedException {
entry.markRebuilding();
assertThrows(
IllegalStateException.class,
() -> setValue(entry, /* value= */ null, /* errorInfo= */ null, /* graphVersion= */ 0L));
() -> setValue(entry, /* value= */ null, /* errorInfo= */ null, initialVersion));
}

@Test
public void crashOnTooManySignals() {
InMemoryNodeEntry entry = createEntry();
entry.addReverseDepAndCheckIfDone(null); // Start evaluation.
entry.markRebuilding();
assertThrows(IllegalStateException.class, () -> entry.signalDep(ZERO_VERSION, null));
assertThrows(IllegalStateException.class, () -> entry.signalDep(initialVersion, null));
}

@Test
public void crashOnSetValueWhenDone() throws InterruptedException {
NodeEntry entry = createEntry();
entry.addReverseDepAndCheckIfDone(null); // Start evaluation.
entry.markRebuilding();
setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 0L);
setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion);
assertThat(entry.isDone()).isTrue();
assertThrows(
IllegalStateException.class,
() -> setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 1L));
() -> setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion));
}

@Test
Expand All @@ -241,8 +241,8 @@ public void forceRebuildLifecycle() throws InterruptedException {
entry.markRebuilding();
SkyKey dep = key("dep");
entry.addSingletonTemporaryDirectDep(dep);
entry.signalDep(ZERO_VERSION, dep);
setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 0L);
entry.signalDep(initialVersion, dep);
setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion);
assertThat(entry.isDirty()).isFalse();
assertThat(entry.isDone()).isTrue();

Expand All @@ -268,18 +268,18 @@ public void forceRebuildLifecycle() throws InterruptedException {

// A force-rebuilt node tolerates evaluating to different values within the same version.
entry.forceRebuild();
assertThat(setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 0L))
assertThat(setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion))
.containsExactly(parent);

assertThat(entry.getVersion()).isEqualTo(ZERO_VERSION);
assertThat(entry.getVersion()).isEqualTo(initialVersion);
}

@Test
public void allowTwiceMarkedForceRebuild() throws InterruptedException {
NodeEntry entry = createEntry();
entry.addReverseDepAndCheckIfDone(null); // Start evaluation.
entry.markRebuilding();
setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 0L);
setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion);
assertThat(entry.isDirty()).isFalse();
assertThat(entry.isDone()).isTrue();
entry.markDirty(DirtyType.FORCE_REBUILD);
Expand All @@ -301,8 +301,7 @@ public void crashOnAddReverseDepTwice() throws InterruptedException {
assertThrows(
"Cannot add same dep twice",
IllegalStateException.class,
() ->
setValue(entry, new SkyValue() {}, /* errorInfo= */ null, /* graphVersion= */ 0L));
() -> setValue(entry, new SkyValue() {}, /* errorInfo= */ null, initialVersion));
assertThat(e).hasMessageThat().containsMatch("[Dd]uplicate( new)? reverse deps");
}

Expand Down Expand Up @@ -398,10 +397,13 @@ public void addTemporaryDirectDepsInGroups_depsExhausted_throws() {
ImmutableSet.of(key("1"), key("2"), key("3")), ImmutableList.of(1, 1, 2)));
}

@CanIgnoreReturnValue
static Set<SkyKey> setValue(
NodeEntry entry, SkyValue value, @Nullable ErrorInfo errorInfo, long graphVersion)
NodeEntry entry, SkyValue value, @Nullable ErrorInfo errorInfo, Version graphVersion)
throws InterruptedException {
return entry.setValue(
ValueWithMetadata.normal(value, errorInfo, NO_EVENTS), IntVersion.of(graphVersion), null);
ValueWithMetadata.normal(value, errorInfo, NO_EVENTS),
checkNotNull(graphVersion),
/* maxTransitiveSourceVersion= */ null);
}
}
Loading

0 comments on commit 53c3518

Please sign in to comment.