Skip to content

Commit

Permalink
Use a ConstantVersion for non-incremental evaluations.
Browse files Browse the repository at this point in the history
This is a prerequisite for not storing a version field in edgeless nodes. In order to satisfy a check in `ParallelEvaluator#informProgressReceiverThatValueIsDone`, the node and the evaluator must agree on the version.

Also skip calling `deleteDirty` if incremental state is not being tracked, since it doesn't make sense with a non-`IntVersion` and is irrelevant without incrementality anyway.

PiperOrigin-RevId: 553489413
Change-Id: Iedd5ad22e99ad3ef7c9398b2105ff289e1fd00b8
  • Loading branch information
justinhorvitz authored and copybara-github committed Aug 3, 2023
1 parent 1fc72f2 commit 87181b8
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,9 @@ protected ExecutionFinishedEvent.Builder createExecutionFinishedEventInternal()
public void deleteOldNodes(long versionWindowForDirtyGc) {
// TODO(bazel-team): perhaps we should come up with a separate GC class dedicated to maintaining
// value garbage. If we ever do so, this logic should be moved there.
memoizingEvaluator.deleteDirty(versionWindowForDirtyGc);
if (trackIncrementalState) {
memoizingEvaluator.deleteDirty(versionWindowForDirtyGc);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,20 @@ public abstract class AbstractIncrementalInMemoryMemoizingEvaluator
protected final GraphInconsistencyReceiver graphInconsistencyReceiver;
protected final EventFilter eventFilter;

// Keep edges in graph. Can be false to save memory, in which case incremental builds are
// not possible.
private final boolean keepEdges;
/**
* Whether to store edges in the graph. Can be false to save memory, in which case incremental
* builds are not possible, and all evaluations will be at {@link ConstantVersion#INSTANCE}.
*/
final boolean keepEdges;

// Values that the caller explicitly specified are assumed to be changed -- they will be
// re-evaluated even if none of their children are changed.
private final InvalidationState invalidatorState = new DirtyingInvalidationState();

protected final EmittedEventState emittedEventState;

protected IntVersion lastGraphVersion = null;
// Null until the first incremental evaluation completes. Always null when not keeping edges.
@Nullable protected IntVersion lastGraphVersion = null;

protected AbstractIncrementalInMemoryMemoizingEvaluator(
ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
Expand Down Expand Up @@ -128,7 +131,7 @@ protected void pruneInjectedValues(Map<SkyKey, Delta> valuesToInject) {
}

/** Injects values in {@code valuesToInject} into the graph. */
protected void injectValues(IntVersion version) {
protected void injectValues(Version version) {
if (valuesToInject.isEmpty()) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
* the returned graphs. However, it is allowed to access the graph from multiple threads as long as
* that does not happen in parallel with an {@link #evaluate} call.
*
* <p>This memoizing evaluator uses a monotonically increasing {@link IntVersion}.
* <p>This memoizing evaluator uses a monotonically increasing {@link IntVersion} for incremental
* evaluations and {@link ConstantVersion#INSTANCE} for non-incremental evaluations.
*/
public final class InMemoryMemoizingEvaluator
extends AbstractIncrementalInMemoryMemoizingEvaluator {
Expand Down Expand Up @@ -126,7 +127,14 @@ public <T extends SkyValue> EvaluationResult<T> evaluate(
Iterable<? extends SkyKey> roots, EvaluationContext evaluationContext)
throws InterruptedException {
// NOTE: Performance critical code. See bug "Null build performance parity".
IntVersion graphVersion = lastGraphVersion == null ? IntVersion.of(0) : lastGraphVersion.next();
Version graphVersion;
if (!keepEdges) {
graphVersion = ConstantVersion.INSTANCE;
} else if (lastGraphVersion == null) {
graphVersion = IntVersion.of(0);
} else {
graphVersion = lastGraphVersion.next();
}
setAndCheckEvaluateState(true, roots);
try {
// Mark for removal any inflight nodes from the previous evaluation.
Expand Down Expand Up @@ -181,7 +189,9 @@ public <T extends SkyValue> EvaluationResult<T> evaluate(
.setWalkableGraph(new DelegatingWalkableGraph(graph))
.build();
} finally {
lastGraphVersion = graphVersion;
if (keepEdges) {
lastGraphVersion = (IntVersion) graphVersion;
}
setAndCheckEvaluateState(false, roots);
}
}
Expand Down

0 comments on commit 87181b8

Please sign in to comment.