diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryNodeEntry.java new file mode 100644 index 00000000000000..70ee46c8a2e764 --- /dev/null +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryNodeEntry.java @@ -0,0 +1,284 @@ +// Copyright 2023 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 static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; + +import com.google.common.base.MoreObjects; +import com.google.common.collect.ImmutableSet; +import java.util.List; +import java.util.Set; +import javax.annotation.Nullable; + +/** + * Partial implementation of {@link InMemoryNodeEntry} containing behavior common to both {@link + * IncrementalInMemoryNodeEntry} and {@link EdgelessInMemoryNodeEntry}. All operations on this class + * are thread-safe. + * + *

Care was taken to provide certain compound operations to avoid certain check-then-act races. + * That means this class is somewhat closely tied to the exact Evaluator implementation. + * + *

Consider the example with two threads working on two nodes, where one depends on the other, + * 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. + * + *

During its life, a node can go through states as follows: + * + *

    + *
  1. Non-existent + *
  2. Just created or marked as affected ({@link #isDone} is false; {@link #isDirty} is false) + *
  3. Evaluating ({@link #isDone} is false; {@link #isDirty} is true) + *
  4. Done ({@link #isDone} is true; {@link #isDirty} is false) + *
+ * + *

The "just created" state is there to allow the {@link ProcessableGraph#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 returns the + * NEEDS_SCHEDULING state only on the first time it was called. A caller that gets NEEDS_SCHEDULING + * back from that call must start the evaluation of this node, while any subsequent callers must + * not. + * + *

An entry is set to ALREADY_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 ALREADY_EVALUATING state until it is DONE. + * + *

From the DONE state, the node can go back to the "marked as affected" state. + */ +abstract class AbstractInMemoryNodeEntry implements InMemoryNodeEntry { + + private final SkyKey key; + + /** Actual data stored in this entry when it is done. */ + @Nullable protected volatile SkyValue value; + + /** + * Tracks state of this entry while it is evaluating (either on its initial build or after being + * marked dirty). + */ + @Nullable protected volatile DirtyBuildingState dirtyBuildingState; + + AbstractInMemoryNodeEntry(SkyKey key) { + this.key = checkNotNull(key); + } + + @Override + public final SkyKey getKey() { + return key; + } + + private boolean isEvaluating() { + return dirtyBuildingState != null; + } + + @Override + public boolean isDone() { + return value != null && dirtyBuildingState == null; + } + + @Override + public final synchronized boolean isReadyToEvaluate() { + return !isDone() + && isEvaluating() + && (dirtyBuildingState.isReady(getNumTemporaryDirectDeps()) + || key.supportsPartialReevaluation()); + } + + @Override + public final synchronized boolean hasUnsignaledDeps() { + checkState(!isDone(), this); + checkState(isEvaluating(), this); + return !dirtyBuildingState.isReady(getNumTemporaryDirectDeps()); + } + + @Override + public synchronized boolean isDirty() { + return !isDone() && dirtyBuildingState != null; + } + + @Override + public synchronized boolean isChanged() { + return !isDone() && dirtyBuildingState != null && dirtyBuildingState.isChanged(); + } + + @Override + public final synchronized SkyValue getValue() { + checkState(isDone(), "no value until done. ValueEntry: %s", this); + return ValueWithMetadata.justValue(value); + } + + @Override + @Nullable + public final SkyValue getValueMaybeWithMetadata() { + return value; + } + + @Override + @Nullable + public final synchronized ErrorInfo getErrorInfo() { + checkState(isDone(), "no errors until done. NodeEntry: %s", this); + return ValueWithMetadata.getMaybeErrorInfo(value); + } + + @Override + public final synchronized void addExternalDep() { + checkNotNull(dirtyBuildingState, this); + dirtyBuildingState.addExternalDep(); + } + + @Override + public final synchronized void forceRebuild() { + checkNotNull(dirtyBuildingState, this); + checkState(isEvaluating(), this); + dirtyBuildingState.forceRebuild(getNumTemporaryDirectDeps()); + } + + @Override + public final synchronized DirtyState getDirtyState() { + checkNotNull(dirtyBuildingState, this); + return dirtyBuildingState.getDirtyState(); + } + + @Override + public final synchronized List getNextDirtyDirectDeps() throws InterruptedException { + checkState(!hasUnsignaledDeps(), this); + checkNotNull(dirtyBuildingState, this); + checkState(dirtyBuildingState.isEvaluating(), "Not evaluating during getNextDirty? %s", this); + return dirtyBuildingState.getNextDirtyDirectDeps(); + } + + @Override + public final synchronized Iterable getAllDirectDepsForIncompleteNode() + throws InterruptedException { + checkState(!isDone(), this); + if (!isDirty()) { + return getTemporaryDirectDeps().getAllElementsAsIterable(); + } else { + // There may be duplicates here. Make sure everything is unique. + ImmutableSet.Builder result = ImmutableSet.builder(); + for (List group : getTemporaryDirectDeps()) { + result.addAll(group); + } + result.addAll( + dirtyBuildingState.getAllRemainingDirtyDirectDeps(/* preservePosition= */ false)); + return result.build(); + } + } + + @Override + public final synchronized ImmutableSet getAllRemainingDirtyDirectDeps() + throws InterruptedException { + checkNotNull(dirtyBuildingState, this); + checkState(dirtyBuildingState.isEvaluating(), "Not evaluating for remaining dirty? %s", this); + if (isDirty()) { + DirtyState dirtyState = dirtyBuildingState.getDirtyState(); + checkState( + dirtyState == DirtyState.REBUILDING || dirtyState == DirtyState.FORCED_REBUILDING, this); + return dirtyBuildingState.getAllRemainingDirtyDirectDeps(/* preservePosition= */ true); + } else { + return ImmutableSet.of(); + } + } + + @Override + public final synchronized void markRebuilding() { + checkNotNull(dirtyBuildingState, this).markRebuilding(); + } + + final GroupedDeps newGroupedDeps() { + // If the key opts into partial reevaluation, tracking deps with a HashSet is worth the extra + // memory cost -- see SkyFunctionEnvironment.PartialReevaluation. + return key.supportsPartialReevaluation() ? new GroupedDeps.WithHashSet() : new GroupedDeps(); + } + + abstract int getNumTemporaryDirectDeps(); + + @Override + public final synchronized boolean noDepsLastBuild() { + checkState(isEvaluating(), this); + return dirtyBuildingState.noDepsLastBuild(); + } + + @Override + public final synchronized void removeUnfinishedDeps(Set unfinishedDeps) { + getTemporaryDirectDeps().remove(unfinishedDeps); + } + + @Override + public final synchronized void addSingletonTemporaryDirectDep(SkyKey dep) { + getTemporaryDirectDeps().appendSingleton(dep); + } + + @Override + public final synchronized void addTemporaryDirectDepGroup(List group) { + getTemporaryDirectDeps().appendGroup(group); + } + + @Override + public final synchronized void addTemporaryDirectDepsInGroups( + Set deps, List groupSizes) { + getTemporaryDirectDeps().appendGroups(deps, groupSizes); + } + + @Override + public final int getPriority() { + var snapshot = dirtyBuildingState; + if (snapshot == null) { + return Integer.MAX_VALUE; + } + return snapshot.getPriority(); + } + + @Override + public final int depth() { + var snapshot = dirtyBuildingState; + if (snapshot == null) { + return 0; + } + return snapshot.depth(); + } + + @Override + public final void updateDepthIfGreater(int proposedDepth) { + var snapshot = dirtyBuildingState; + if (snapshot == null) { + return; + } + snapshot.updateDepthIfGreater(proposedDepth); + } + + @Override + public final void incrementEvaluationCount() { + var snapshot = dirtyBuildingState; + if (snapshot == null) { + return; + } + snapshot.incrementEvaluationCount(); + } + + protected synchronized MoreObjects.ToStringHelper toStringHelper() { + return MoreObjects.toStringHelper(this) + .add("key", key) + .add("value", value) + .add("dirtyBuildingState", dirtyBuildingState); + } + + @Override + public final synchronized String toString() { + return toStringHelper().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 b630b34a719aa8..38d8b1d838d5c6 100644 --- a/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java +++ b/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java @@ -205,7 +205,8 @@ private void checkFinishedBuildingWhenAboutToSetValue() { * DirtyBuildingState#getNumOfGroupsInLastBuildDirectDeps()}. */ final void signalDep( - IncrementalInMemoryNodeEntry entry, + AbstractInMemoryNodeEntry entry, + NodeVersion version, Version childVersion, @Nullable SkyKey childForDebugging) { // Synchronization isn't needed here because the only caller is InMemoryNodeEntry, which does it @@ -217,7 +218,7 @@ final void signalDep( } // childVersion > version.lastEvaluated() means the child has changed since the last evaluation. - boolean childChanged = !childVersion.atMost(entry.version.lastEvaluated()); + boolean childChanged = !childVersion.atMost(version.lastEvaluated()); if (childChanged) { dirtyState = DirtyState.NEEDS_REBUILDING; } else if (dirtyState == DirtyState.CHECK_DEPENDENCIES diff --git a/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java index c2fba7989996de..5e345763490d18 100644 --- a/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java @@ -18,7 +18,6 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.skyframe.KeyToConsolidate.Op; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.ForOverride; @@ -28,50 +27,8 @@ import java.util.Set; import javax.annotation.Nullable; -/** - * In-memory implementation of {@link NodeEntry}. All operations on this class are thread-safe. - * - *

Care was taken to provide certain compound operations to avoid certain check-then-act races. - * That means this class is somewhat closely tied to the exact Evaluator implementation. - * - *

Consider the example with two threads working on two nodes, where one depends on the other, - * 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. - * - *

During its life, a node can go through states as follows: - * - *

    - *
  1. Non-existent - *
  2. Just created or marked as affected ({@link #isDone} is false; {@link #isDirty} is false) - *
  3. Evaluating ({@link #isDone} is false; {@link #isDirty} is true) - *
  4. Done ({@link #isDone} is true; {@link #isDirty} is false) - *
- * - *

The "just created" state is there to allow the {@link ProcessableGraph#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 returns the - * NEEDS_SCHEDULING state only on the first time it was called. A caller that gets NEEDS_SCHEDULING - * back from that call must start the evaluation of this node, while any subsequent callers must - * not. - * - *

An entry is set to ALREADY_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 ALREADY_EVALUATING state until it is DONE. - * - *

From the DONE state, the node can go back to the "marked as affected" state. - * - *

This class is public only for the benefit of alternative graph implementations outside of the - * package. - */ -public class IncrementalInMemoryNodeEntry implements InMemoryNodeEntry { - - private final SkyKey key; - - /** Actual data stored in this entry when it is done. */ - protected volatile SkyValue value = null; +/** An {@link InMemoryNodeEntry} that {@link #keepsEdges} for use in incremental evaluations. */ +public class IncrementalInMemoryNodeEntry extends AbstractInMemoryNodeEntry { protected volatile NodeVersion version = Version.minimal(); @@ -121,19 +78,8 @@ public class IncrementalInMemoryNodeEntry implements InMemoryNodeEntry { */ private List reverseDepsDataToConsolidate = null; - /** - * Object encapsulating dirty state of the object between when it is marked dirty and - * re-evaluated. - */ - @Nullable protected volatile DirtyBuildingState dirtyBuildingState = null; - public IncrementalInMemoryNodeEntry(SkyKey key) { - this.key = checkNotNull(key); - } - - @Override - public final SkyKey getKey() { - return key; + super(key); } @Override @@ -141,52 +87,6 @@ public boolean keepsEdges() { return true; } - private boolean isEvaluating() { - return dirtyBuildingState != null; - } - - @Override - public boolean isDone() { - return value != null && dirtyBuildingState == null; - } - - @Override - public synchronized boolean isReadyToEvaluate() { - return !isDone() - && isEvaluating() - && (dirtyBuildingState.isReady(getNumTemporaryDirectDeps()) - || key.supportsPartialReevaluation()); - } - - @Override - public synchronized boolean hasUnsignaledDeps() { - checkState(!isDone(), this); - checkState(isEvaluating(), this); - return !dirtyBuildingState.isReady(getNumTemporaryDirectDeps()); - } - - @Override - public synchronized boolean isDirty() { - return !isDone() && dirtyBuildingState != null; - } - - @Override - public synchronized boolean isChanged() { - return !isDone() && dirtyBuildingState != null && dirtyBuildingState.isChanged(); - } - - @Override - public synchronized SkyValue getValue() { - checkState(isDone(), "no value until done. ValueEntry: %s", this); - return ValueWithMetadata.justValue(value); - } - - @Override - @Nullable - public SkyValue getValueMaybeWithMetadata() { - return value; - } - @Nullable @Override public SkyValue toValue() { @@ -228,27 +128,15 @@ public boolean hasAtLeastOneDep() { return GroupedDeps.castAsCompressed(directDeps); } - @Override - @Nullable - public synchronized ErrorInfo getErrorInfo() { - checkState(isDone(), "no errors until done. NodeEntry: %s", this); - return ValueWithMetadata.getMaybeErrorInfo(value); - } - /** * Puts entry in "done" state, as checked by {@link #isDone}. Subclasses that override one may * need to override the other. */ + @ForOverride protected void markDone() { dirtyBuildingState = null; } - @Override - public synchronized void addExternalDep() { - checkNotNull(dirtyBuildingState, this); - dirtyBuildingState.addExternalDep(); - } - protected final synchronized Set setStateFinishedAndReturnReverseDepsToSignal() { Set reverseDepsToSignal = ReverseDepsUtility.consolidateDataAndReturnNewElements(this); directDeps = keepsEdges() ? getTemporaryDirectDeps().compress() : null; @@ -306,7 +194,7 @@ public DependencyState addReverseDepAndCheckIfDone(SkyKey reverseDep) { synchronized (this) { boolean done = isDone(); if (!done && dirtyBuildingState == null) { - dirtyBuildingState = new InitialBuildingState(key.hasLowFanout()); + dirtyBuildingState = new InitialBuildingState(getKey().hasLowFanout()); } if (reverseDep != null) { if (done) { @@ -416,7 +304,7 @@ public synchronized boolean signalDep(Version childVersion, @Nullable SkyKey chi checkState( !isDone(), "Value must not be done in signalDep %s child=%s", this, childForDebugging); checkNotNull(dirtyBuildingState, "%s %s", this, childForDebugging); - dirtyBuildingState.signalDep(this, childVersion, childForDebugging); + dirtyBuildingState.signalDep(this, version, childVersion, childForDebugging); return !hasUnsignaledDeps(); } @@ -432,7 +320,7 @@ private void assertKeepEdges() { @ForOverride protected DirtyBuildingState createDirtyBuildingStateForDoneNode( DirtyType dirtyType, GroupedDeps directDeps, SkyValue value) { - return new IncrementalDirtyBuildingState(dirtyType, key, directDeps, value); + return new IncrementalDirtyBuildingState(dirtyType, getKey(), directDeps, value); } private static final GroupedDeps EMPTY_LIST = new GroupedDeps(); @@ -494,102 +382,26 @@ public synchronized NodeValueAndRdepsToSignal markClean() throws InterruptedExce return new NodeValueAndRdepsToSignal(this.value, rDepsToSignal); } - @Override - public synchronized void forceRebuild() { - checkNotNull(dirtyBuildingState, this); - checkState(isEvaluating(), this); - dirtyBuildingState.forceRebuild(getNumTemporaryDirectDeps()); - } - @Override public Version getVersion() { return version.lastChanged(); } - @Override - public synchronized NodeEntry.DirtyState getDirtyState() { - checkNotNull(dirtyBuildingState, this); - return dirtyBuildingState.getDirtyState(); - } - - /** - * @see DirtyBuildingState#getNextDirtyDirectDeps() - */ - @Override - public synchronized List getNextDirtyDirectDeps() throws InterruptedException { - checkState(!hasUnsignaledDeps(), this); - checkNotNull(dirtyBuildingState, this); - checkState(dirtyBuildingState.isEvaluating(), "Not evaluating during getNextDirty? %s", this); - return dirtyBuildingState.getNextDirtyDirectDeps(); - } - - @Override - public synchronized Iterable getAllDirectDepsForIncompleteNode() - throws InterruptedException { - checkState(!isDone(), this); - if (!isDirty()) { - return getTemporaryDirectDeps().getAllElementsAsIterable(); - } else { - // There may be duplicates here. Make sure everything is unique. - ImmutableSet.Builder result = ImmutableSet.builder(); - for (List group : getTemporaryDirectDeps()) { - result.addAll(group); - } - result.addAll( - dirtyBuildingState.getAllRemainingDirtyDirectDeps(/* preservePosition= */ false)); - return result.build(); - } - } - - @Override - public synchronized ImmutableSet getAllRemainingDirtyDirectDeps() - throws InterruptedException { - checkNotNull(dirtyBuildingState, this); - checkState(dirtyBuildingState.isEvaluating(), "Not evaluating for remaining dirty? %s", this); - if (isDirty()) { - DirtyState dirtyState = dirtyBuildingState.getDirtyState(); - checkState( - dirtyState == DirtyState.REBUILDING || dirtyState == DirtyState.FORCED_REBUILDING, this); - return dirtyBuildingState.getAllRemainingDirtyDirectDeps(/* preservePosition= */ true); - } else { - return ImmutableSet.of(); - } - } - - @Override - public synchronized void markRebuilding() { - checkNotNull(dirtyBuildingState, this).markRebuilding(); - } - @Override public synchronized GroupedDeps getTemporaryDirectDeps() { checkState(!isDone(), "temporary shouldn't be done: %s", this); if (directDeps == null) { // Initialize lazily, to save a little memory. - // - // If the key opts into partial reevaluation, tracking deps with a HashSet is worth the extra - // memory cost -- see SkyFunctionEnvironment.PartialReevaluation. - directDeps = - key.supportsPartialReevaluation() ? new GroupedDeps.WithHashSet() : new GroupedDeps(); + directDeps = newGroupedDeps(); } return (GroupedDeps) directDeps; } + @Override final synchronized int getNumTemporaryDirectDeps() { return directDeps == null ? 0 : getTemporaryDirectDeps().numElements(); } - @Override - public synchronized boolean noDepsLastBuild() { - checkState(isEvaluating(), this); - return dirtyBuildingState.noDepsLastBuild(); - } - - @Override - public synchronized void removeUnfinishedDeps(Set unfinishedDeps) { - getTemporaryDirectDeps().remove(unfinishedDeps); - } - @Override public synchronized void resetForRestartFromScratch() { checkState(!hasUnsignaledDeps(), this); @@ -598,75 +410,15 @@ public synchronized void resetForRestartFromScratch() { } @Override - public synchronized void addSingletonTemporaryDirectDep(SkyKey dep) { - getTemporaryDirectDeps().appendSingleton(dep); - } - - @Override - public synchronized void addTemporaryDirectDepGroup(List group) { - getTemporaryDirectDeps().appendGroup(group); - } - - @Override - public synchronized void addTemporaryDirectDepsInGroups( - Set deps, List groupSizes) { - getTemporaryDirectDeps().appendGroups(deps, groupSizes); - } - - @Override - public int getPriority() { - var snapshot = dirtyBuildingState; - if (snapshot == null) { - return Integer.MAX_VALUE; - } - return snapshot.getPriority(); - } - - @Override - public int depth() { - var snapshot = dirtyBuildingState; - if (snapshot == null) { - return 0; - } - return snapshot.depth(); - } - - @Override - public void updateDepthIfGreater(int proposedDepth) { - var snapshot = dirtyBuildingState; - if (snapshot == null) { - return; - } - snapshot.updateDepthIfGreater(proposedDepth); - } - - @Override - public void incrementEvaluationCount() { - var snapshot = dirtyBuildingState; - if (snapshot == null) { - return; - } - snapshot.incrementEvaluationCount(); - } - protected synchronized MoreObjects.ToStringHelper toStringHelper() { - return MoreObjects.toStringHelper(this) - .add("key", key) - .add("identity", System.identityHashCode(this)) - .add("value", value) + return super.toStringHelper() .add("version", version) .add( "directDeps", isDone() && keepsEdges() ? GroupedDeps.decompress(getCompressedDirectDepsForDoneEntry()) : directDeps) - .add("reverseDeps", ReverseDepsUtility.toString(this)) - .add("dirtyBuildingState", dirtyBuildingState); - } - - @Override - public final synchronized String toString() { - return toStringHelper().toString(); + .add("reverseDeps", ReverseDepsUtility.toString(this)); } /** {@link DirtyBuildingState} for a node on an incremental build. */