Add events and get rid of ErrorInfoEncoder. Clean up some signatures and visibility in Skyframe classes. PiperOrigin-RevId: 197665817
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DeserializationContext.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DeserializationContext.java index 45bde75..7a3dcae 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DeserializationContext.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/DeserializationContext.java
@@ -106,7 +106,7 @@ * context. */ @CheckReturnValue - DeserializationContext getMemoizingContext() { + public DeserializationContext getMemoizingContext() { if (deserializer != null) { return this; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationContext.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationContext.java index c16c647..dedbf2d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationContext.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationContext.java
@@ -102,7 +102,7 @@ * context. */ @CheckReturnValue - SerializationContext getMemoizingContext() { + public SerializationContext getMemoizingContext() { if (serializer != null) { return this; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationResult.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationResult.java index a22f546..90a1598 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationResult.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationResult.java
@@ -15,9 +15,7 @@ package com.google.devtools.build.lib.skyframe.serialization; import com.google.common.base.Preconditions; -import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; -import com.google.common.util.concurrent.MoreExecutors; import com.google.protobuf.ByteString; import javax.annotation.Nullable; @@ -45,13 +43,6 @@ public abstract <S> SerializationResult<S> with(S newObj); /** - * Returns a new {@link SerializationResult} with the same object but with a new future that waits - * on the given future as well as the current future. - */ - public abstract SerializationResult<T> withAdditionalFuture( - ListenableFuture<Void> additionalFuture); - - /** * Returns a {@link ListenableFuture} that, if not null, must complete successfully before {@link * #getObject} can be written remotely. */ @@ -86,11 +77,6 @@ } @Override - public SerializationResult<T> withAdditionalFuture(ListenableFuture<Void> additionalFuture) { - return new ObjectWithFuture<>(getObject(), additionalFuture); - } - - @Override public ListenableFuture<Void> getFutureToBlockWritesOn() { return null; } @@ -110,14 +96,6 @@ } @Override - public SerializationResult<T> withAdditionalFuture(ListenableFuture<Void> additionalFuture) { - ListenableFuture<Void> combinedFuture = - Futures.whenAllComplete(additionalFuture, futureToBlockWritesOn) - .call(() -> null, MoreExecutors.directExecutor()); - return new ObjectWithFuture<>(getObject(), combinedFuture); - } - - @Override public ListenableFuture<Void> getFutureToBlockWritesOn() { return futureToBlockWritesOn; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java index c8fda47..69433b1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/SerializationTester.java
@@ -58,7 +58,7 @@ void verifyDeserialized(T original, T deserialized) throws Exception; } - private final ImmutableList<Object> subjects; + private final ImmutableList<?> subjects; private final ImmutableMap.Builder<Class<?>, Object> dependenciesBuilder; private final ArrayList<ObjectCodec<?>> additionalCodecs = new ArrayList<>(); private boolean memoize; @@ -75,7 +75,7 @@ this(ImmutableList.copyOf(subjects)); } - public SerializationTester(ImmutableList<Object> subjects) { + public SerializationTester(ImmutableList<?> subjects) { Preconditions.checkArgument(!subjects.isEmpty()); this.subjects = subjects; this.dependenciesBuilder = ImmutableMap.builder();
diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java index 82d58ea..7a2c416 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java +++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
@@ -56,7 +56,7 @@ ImmutableList.of(cycleInfo), /*isDirectlyTransient=*/ false, /*isTransitivelyTransient=*/ false, - /* isCatostrophic= */ false); + /* isCatastrophic= */ false); } /** Create an ErrorInfo from a collection of existing errors. */ @@ -110,7 +110,7 @@ ImmutableList<CycleInfo> cycles, boolean isDirectlyTransient, boolean isTransitivelyTransient, - boolean isCatostrophic) { + boolean isCatastrophic) { Preconditions.checkState(exception != null || !Iterables.isEmpty(cycles), "At least one of exception and cycles must be non-null/empty, respectively"); Preconditions.checkState((exception == null) == (rootCauseOfException == null), @@ -123,7 +123,7 @@ this.cycles = cycles; this.isDirectlyTransient = isDirectlyTransient; this.isTransitivelyTransient = isTransitivelyTransient; - this.isCatastrophic = isCatostrophic; + this.isCatastrophic = isCatastrophic; } @Override @@ -234,7 +234,7 @@ * path is returned here. However, if there are multiple paths to the same cycle, each of which * goes through a different child, each of them is returned here. */ - public Iterable<CycleInfo> getCycleInfo() { + public ImmutableList<CycleInfo> getCycleInfo() { return cycles; }
diff --git a/src/main/java/com/google/devtools/build/skyframe/EventFilter.java b/src/main/java/com/google/devtools/build/skyframe/EventFilter.java index 113e9f3..0b859d2 100644 --- a/src/main/java/com/google/devtools/build/skyframe/EventFilter.java +++ b/src/main/java/com/google/devtools/build/skyframe/EventFilter.java
@@ -19,8 +19,8 @@ /** Filters out events which should not be stored during evaluation in {@link ParallelEvaluator}. */ public interface EventFilter extends Predicate<Event> { /** - * Returns true if any events should be stored. Otherwise, optimizations may be made to avoid - * doing unnecessary work when evaluating node entries. + * Returns true if any events/postables should be stored. Otherwise, optimizations may be made to + * avoid doing unnecessary work when evaluating node entries. */ - boolean storeEvents(); + boolean storeEventsAndPosts(); }
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java index a035c03..a1f6461 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -376,6 +376,7 @@ public ImmutableMap<SkyFunctionName, ? extends SkyFunction> getSkyFunctionsForTesting() { return skyFunctions; } + public static final EventFilter DEFAULT_STORED_EVENT_FILTER = new EventFilter() { @Override @@ -390,7 +391,7 @@ } @Override - public boolean storeEvents() { + public boolean storeEventsAndPosts() { return true; } };
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java index 920c6c1..905e822 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -24,6 +24,7 @@ import com.google.common.collect.Sets; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; @@ -190,6 +191,9 @@ NestedSet<TaggedEvents> buildEvents(NodeEntry entry, boolean missingChildren) throws InterruptedException { + if (!evaluatorContext.getStoredEventFilter().storeEventsAndPosts()) { + return NestedSetBuilder.emptySet(Order.STABLE_ORDER); + } // Aggregate the nested set of events from the direct deps, also adding the events from // building this value. NestedSetBuilder<TaggedEvents> eventBuilder = NestedSetBuilder.stableOrder(); @@ -197,29 +201,29 @@ if (!events.isEmpty()) { eventBuilder.add(new TaggedEvents(getTagFromKey(), events)); } - if (evaluatorContext.getStoredEventFilter().storeEvents()) { - // Only do the work of processing children if we're going to store events. - GroupedList<SkyKey> depKeys = entry.getTemporaryDirectDeps(); - Collection<SkyValue> deps = getDepValuesForDoneNodeMaybeFromError(depKeys); - if (!missingChildren && depKeys.numElements() != deps.size()) { - throw new IllegalStateException( - "Missing keys for " - + skyKey - + ". Present values: " - + deps - + " requested from: " - + depKeys - + ", " - + entry); - } - for (SkyValue value : deps) { - eventBuilder.addTransitive(ValueWithMetadata.getEvents(value)); - } + GroupedList<SkyKey> depKeys = entry.getTemporaryDirectDeps(); + Collection<SkyValue> deps = getDepValuesForDoneNodeMaybeFromError(depKeys); + if (!missingChildren && depKeys.numElements() != deps.size()) { + throw new IllegalStateException( + "Missing keys for " + + skyKey + + ". Present values: " + + deps + + " requested from: " + + depKeys + + ", " + + entry); + } + for (SkyValue value : deps) { + eventBuilder.addTransitive(ValueWithMetadata.getEvents(value)); } return eventBuilder.build(); } NestedSet<Postable> buildPosts(NodeEntry entry) throws InterruptedException { + if (!evaluatorContext.getStoredEventFilter().storeEventsAndPosts()) { + return NestedSetBuilder.emptySet(Order.STABLE_ORDER); + } NestedSetBuilder<Postable> postBuilder = NestedSetBuilder.stableOrder(); postBuilder.addAll(eventHandler.getPosts());
diff --git a/src/main/java/com/google/devtools/build/skyframe/TaggedEvents.java b/src/main/java/com/google/devtools/build/skyframe/TaggedEvents.java index 0d3e422..9c20763 100644 --- a/src/main/java/com/google/devtools/build/skyframe/TaggedEvents.java +++ b/src/main/java/com/google/devtools/build/skyframe/TaggedEvents.java
@@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.skyframe; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; @@ -36,7 +37,8 @@ private final ImmutableList<Event> events; private final int hashCode; - TaggedEvents(final @Nullable String tag, ImmutableCollection<Event> events) { + @VisibleForTesting + public TaggedEvents(final @Nullable String tag, ImmutableCollection<Event> events) { this.events = events.isEmpty() ? ImmutableList.of()
diff --git a/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java b/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java index ce92dfd..a0c5108 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java +++ b/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
@@ -12,7 +12,10 @@ // See the License for the specific language governing permissions and package com.google.devtools.build.skyframe; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -34,7 +37,7 @@ private static final NestedSet<Postable> NO_POSTS = NestedSetBuilder.<Postable>emptySet(Order.STABLE_ORDER); - public ValueWithMetadata(SkyValue value) { + private ValueWithMetadata(SkyValue value) { this.value = value; } @@ -47,12 +50,12 @@ ErrorInfo errorInfo, NestedSet<TaggedEvents> transitiveEvents, NestedSet<Postable> transitivePostables) { - return new ErrorInfoValue(errorInfo, null, transitiveEvents, transitivePostables); + return (ValueWithMetadata) normal(null, errorInfo, transitiveEvents, transitivePostables); } /** - * Builds a value entry value that has a value value, and possibly an error (constructed from its - * children's errors). + * Builds a SkyValue that has a value, and possibly an error, and possibly events/postables. If it + * has only a value, returns just the value in order to save memory. * * <p>This is public only for use in alternative {@code MemoizingEvaluator} implementations. */ @@ -83,8 +86,8 @@ public abstract NestedSet<Postable> getTransitivePostables(); /** Implementation of {@link ValueWithMetadata} for the value case. */ + @VisibleForTesting public static class ValueWithEvents extends ValueWithMetadata { - private final NestedSet<TaggedEvents> transitiveEvents; private final NestedSet<Postable> transitivePostables; @@ -97,7 +100,7 @@ this.transitivePostables = Preconditions.checkNotNull(transitivePostables); } - public static ValueWithEvents createValueWithEvents( + private static ValueWithEvents createValueWithEvents( SkyValue value, NestedSet<TaggedEvents> transitiveEvents, NestedSet<Postable> transitivePostables) { @@ -154,7 +157,13 @@ } @Override - public String toString() { return value.toString(); } + public String toString() { + return MoreObjects.toStringHelper(this) + .add("value", value) + .add("transitiveEvents size", Iterables.size(transitiveEvents)) + .add("transitivePostables size", Iterables.size(transitivePostables)) + .toString(); + } } private static final class NotComparableValueWithEvents extends ValueWithEvents
diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java index f2b5043..8fd7287 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -558,7 +558,7 @@ } @Override - public boolean storeEvents() { + public boolean storeEventsAndPosts() { return true; } });