Add events and get rid of ErrorInfoEncoder. Clean up some signatures and visibility in Skyframe classes.

PiperOrigin-RevId: 197665817

11 files changed, 55 insertions, 61 deletions

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 45bde757b8..7a3dcaec10 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 @@ public class DeserializationContext {
    * 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 c16c6479e6..dedbf2dab7 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 @@ public class SerializationContext {
    * 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 a22f546c85..90a1598700 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 class SerializationResult<T> {
   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 @@ public abstract class SerializationResult<T> {
     }
 
     @Override
-    public SerializationResult<T> withAdditionalFuture(ListenableFuture<Void> additionalFuture) {
-      return new ObjectWithFuture<>(getObject(), additionalFuture);
-    }
-
-    @Override
     public ListenableFuture<Void> getFutureToBlockWritesOn() {
       return null;
     }
@@ -110,14 +96,6 @@ public abstract class SerializationResult<T> {
     }
 
     @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 c8fda47539..69433b10dc 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 @@ public class SerializationTester {
     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 @@ public class SerializationTester {
     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 82d58ea19e..7a2c4160b3 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 @@ public class ErrorInfo {
         ImmutableList.of(cycleInfo),
         /*isDirectlyTransient=*/ false,
         /*isTransitivelyTransient=*/ false,
-        /* isCatostrophic= */ false);
+        /* isCatastrophic= */ false);
   }
 
   /** Create an ErrorInfo from a collection of existing errors. */
@@ -110,7 +110,7 @@ public class ErrorInfo {
       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 @@ public class ErrorInfo {
     this.cycles = cycles;
     this.isDirectlyTransient = isDirectlyTransient;
     this.isTransitivelyTransient = isTransitivelyTransient;
-    this.isCatastrophic = isCatostrophic;
+    this.isCatastrophic = isCatastrophic;
   }
 
   @Override
@@ -234,7 +234,7 @@ public class ErrorInfo {
    * 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 113e9f3548..0b859d24ff 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 @@ import com.google.devtools.build.lib.events.Event;
 /** 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 a035c0374c..a1f6461e50 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 final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
   public ImmutableMap<SkyFunctionName, ? extends SkyFunction> getSkyFunctionsForTesting() {
     return skyFunctions;
   }
+
   public static final EventFilter DEFAULT_STORED_EVENT_FILTER =
       new EventFilter() {
         @Override
@@ -390,7 +391,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
         }
 
         @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 920c6c18d8..905e822c57 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.Maps;
 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
 
   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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
     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 0d3e422b65..9c20763a4e 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 @@ public final class TaggedEvents implements Serializable {
   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 ce92dfd2b2..a0c5108590 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 @@ public abstract class ValueWithMetadata implements SkyValue {
   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 @@ public abstract class ValueWithMetadata implements SkyValue {
       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 class ValueWithMetadata implements SkyValue {
   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 @@ public abstract class ValueWithMetadata implements SkyValue {
       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 @@ public abstract class ValueWithMetadata implements SkyValue {
     }
 
     @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 f2b5043a77..8fd7287431 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 @@ public class ParallelEvaluatorTest {
               }
 
               @Override
-              public boolean storeEvents() {
+              public boolean storeEventsAndPosts() {
                 return true;
               }
             });