Sure up handling of events and posts from sky functions.

This change addresses all of the oddities described in b/239458245 with fixes and/or documentation. `Event` and `Postable` now share a common interface `Reportable` which has a `storeForReplay` method which dictates the semantics of how they should be handled (documented in `SkyFunction.Environment` and tested in `ParallelEvaluatorTest`).

Skyframe no longer distinguishes between events and posts - all `Reportable` instances are handled opaquely and stored together. `EventFilter` no longer acts as a per-event predicate - this logic was always the same if events were being stored at all, so it is moved to `Event#storeForReplay`.

Some tests which trigger error events in a loop had to switch from `invalidatePackages()` to `initializeSkyframeExecutor()`. The latter additionally clears the emitted event state, which is now keyed on an individual event instead of the `TaggedEvents` list, and those test cases had multiple identical events which were getting cached between loop iterations with the new strategy.

PiperOrigin-RevId: 463222076
Change-Id: Ib6e6acf29bc99e6744fae992166ad06d66ab5b37

src/main/java/com/google/devtools/build/lib/events/Event.java

@@ -38,7 +38,7 @@
  */
 @Immutable
 @CheckReturnValue
-public final class Event {
+public final class Event implements Reportable {
 
   private final EventKind kind;
 
@@ -66,6 +66,16 @@ private Event(EventKind kind, Object message, ImmutableClassToInstanceMap<Object
     this.properties = checkNotNull(properties);
   }
 
+  @Override
+  public void reportTo(ExtendedEventHandler handler) {
+    handler.handle(this);
+  }
+
+  @Override
+  public boolean storeForReplay() {
+    return kind != EventKind.PROGRESS && kind != EventKind.INFO;
+  }
+
   public EventKind getKind() {
     return kind;
   }
@@ -151,12 +161,15 @@ private <T> void addToBuilder(ImmutableClassToInstanceMap.Builder<Object> builde
   }
 
   /**
-   * Like {@link #withProperty(Class, Object)}, with {@code type.equals(String.class)}.
+   * {@inheritDoc}
+   *
+   * <p>Behaves like {@link #withProperty(Class, Object)}, with {@code type.equals(String.class)}.
    *
    * <p>Additionally, if the event this is called on already has a {@link String} property with
    * value {@code tag}, or if {@code tag} is {@code null} and the event has no {@link String}
    * property, then this returns that event (it does not create a new {@link Event} instance).
    */
+  @Override
   public Event withTag(@Nullable String tag) {
     if (Objects.equals(tag, getProperty(String.class))) {
       return this;

src/main/java/com/google/devtools/build/lib/events/ExtendedEventHandler.java

@@ -14,31 +14,25 @@
 
 package com.google.devtools.build.lib.events;
 
+import javax.annotation.Nullable;
+
 /**
  * Interface for reporting events during the build. It extends the {@link EventHandler} by also
  * allowing posting more structured information.
  */
 public interface ExtendedEventHandler extends EventHandler {
 
   /** An event that can be posted via the extended event handler. */
-  interface Postable {
+  interface Postable extends Reportable {
 
-    /**
-     * If this post originated from {@link
-     * com.google.devtools.build.skyframe.SkyFunction.Environment#getListener}, whether it should be
-     * stored in the corresponding Skyframe node to be replayed on incremental builds when the node
-     * is deemed up-to-date.
-     *
-     * <p>Posts which are crucial to the correctness of the evaluation should return {@code true} so
-     * that they are replayed when the {@link com.google.devtools.build.skyframe.SkyFunction}
-     * invocation is cached. On the other hand, posts that are merely informational (such as a
-     * progress update) should return {@code false} to avoid taking up memory.
-     *
-     * <p>This method is not relevant for posts which do not originate from {@link
-     * com.google.devtools.build.skyframe.SkyFunction} evaluation.
-     */
-    default boolean storeForReplay() {
-      return false;
+    @Override
+    default void reportTo(ExtendedEventHandler handler) {
+      handler.post(this);
+    }
+
+    @Override
+    default Postable withTag(@Nullable String tag) {
+      return this; // No tag-based filtering.
     }
   }
 

src/main/java/com/google/devtools/build/lib/events/Reportable.java

@@ -0,0 +1,54 @@
+// Copyright 2022 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.lib.events;
+
+import javax.annotation.Nullable;
+
+/** An event that can be reported to an {@link ExtendedEventHandler}. */
+public interface Reportable {
+
+  void reportTo(ExtendedEventHandler handler);
+
+  /**
+   * If this event supports tag-based output filtering, returns a new instance identical to this one
+   * but with the given tag. Otherwise returns {@code this}.
+   *
+   * <p>Tags can be used to apply filtering to events. See {@link OutputFilter}.
+   */
+  Reportable withTag(@Nullable String tag);
+
+  /**
+   * If this event originated from {@link
+   * com.google.devtools.build.skyframe.SkyFunction.Environment#reportEvent}, whether it should be
+   * stored in the corresponding Skyframe node to be replayed on incremental builds when the node is
+   * deemed up-to-date.
+   *
+   * <p>Events which are crucial to the correctness of the evaluation should return {@code true} so
+   * that they are replayed when the {@link com.google.devtools.build.skyframe.SkyFunction}
+   * invocation is cached. On the other hand, events that are merely informational (such as a
+   * progress update) should return {@code false} to avoid taking up memory.
+   *
+   * <p>Evaluations may disable all event storage and replay by using a custom {@link
+   * com.google.devtools.build.skyframe.EventFilter}, in which case this method is only used to
+   * fulfill the semantics of {@link
+   * com.google.devtools.build.skyframe.SkyFunction.Environment#reportEvent}.
+   *
+   * <p>This method is not relevant for events which do not originate from {@link
+   * com.google.devtools.build.skyframe.SkyFunction} evaluation.
+   */
+  default boolean storeForReplay() {
+    return false;
+  }
+}

src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java

@@ -44,6 +44,7 @@
 import com.google.devtools.build.lib.bugreport.BugReporter;
 import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor;
 import com.google.devtools.build.lib.concurrent.Uninterruptibles;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
@@ -92,7 +93,6 @@
 import com.google.devtools.build.skyframe.GraphInconsistencyReceiver;
 import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
 import com.google.devtools.build.skyframe.Injectable;
-import com.google.devtools.build.skyframe.MemoizingEvaluator.EmittedEventState;
 import com.google.devtools.build.skyframe.RecordingDifferencer;
 import com.google.devtools.build.skyframe.SequencedRecordingDifferencer;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -217,7 +217,7 @@ protected InMemoryMemoizingEvaluator createEvaluator(
       ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
       SkyframeProgressReceiver progressReceiver,
       EventFilter eventFilter,
-      EmittedEventState emittedEventState) {
+      NestedSetVisitor.VisitedState emittedEventState) {
     return new InMemoryMemoizingEvaluator(
         skyFunctions,
         recordingDiffer,

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -118,6 +118,7 @@
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor;
 import com.google.devtools.build.lib.concurrent.ExecutorUtil;
 import com.google.devtools.build.lib.concurrent.NamedForkJoinPool;
 import com.google.devtools.build.lib.concurrent.Sharder;
@@ -204,11 +205,9 @@
 import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.EventFilter;
 import com.google.devtools.build.skyframe.ImmutableDiff;
-import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
 import com.google.devtools.build.skyframe.InMemoryNodeEntry;
 import com.google.devtools.build.skyframe.Injectable;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
-import com.google.devtools.build.skyframe.MemoizingEvaluator.EmittedEventState;
 import com.google.devtools.build.skyframe.NodeEntry;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -257,8 +256,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur
   private static final int PARALLELISM_THRESHOLD = 1024;
 
   protected MemoizingEvaluator memoizingEvaluator;
-  private final MemoizingEvaluator.EmittedEventState emittedEventState =
-      new MemoizingEvaluator.EmittedEventState();
+  private final NestedSetVisitor.VisitedState emittedEventState =
+      new NestedSetVisitor.VisitedState();
   protected final PackageFactory pkgFactory;
   private final WorkspaceStatusAction.Factory workspaceStatusActionFactory;
   private final FileSystem fileSystem;
@@ -650,8 +649,7 @@ private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions() {
         new BuildDriverFunction(
             new TransitiveActionLookupValuesHelper() {
               @Override
-              public ActionLookupValuesCollectionResult collect(ActionLookupKey key)
-                  throws InterruptedException {
+              public ActionLookupValuesCollectionResult collect(ActionLookupKey key) {
                 return collectTransitiveActionLookupValues(key);
               }
 
@@ -811,7 +809,7 @@ protected abstract MemoizingEvaluator createEvaluator(
       ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
       SkyframeProgressReceiver progressReceiver,
       EventFilter eventFilter,
-      EmittedEventState emittedEventState);
+      NestedSetVisitor.VisitedState emittedEventState);
 
   /**
    * Use the fact that analysis of a target must occur before execution of that target, and in a
@@ -823,16 +821,10 @@ protected abstract MemoizingEvaluator createEvaluator(
   private static final EventFilter DEFAULT_FILTER_WITH_ACTIONS =
       new EventFilter() {
         @Override
-        public boolean storeEventsAndPosts() {
+        public boolean storeEvents() {
           return true;
         }
 
-        @Override
-        public boolean test(Event input) {
-          // Use the filtering defined in the default filter: no info/progress messages.
-          return InMemoryMemoizingEvaluator.DEFAULT_STORED_EVENT_FILTER.test(input);
-        }
-
         @Override
         public boolean shouldPropagate(SkyKey depKey, SkyKey primaryKey) {
           // Do not propagate events from analysis phase nodes to execution phase nodes.

src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java

@@ -33,6 +33,7 @@
 import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue;
 import com.google.devtools.build.lib.clock.BlazeClock;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor;
 import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
 import com.google.devtools.build.lib.concurrent.ExecutorUtil;
 import com.google.devtools.build.lib.concurrent.NamedForkJoinPool;
@@ -90,6 +91,7 @@
 import com.google.devtools.build.skyframe.EvaluationContext;
 import com.google.devtools.build.skyframe.EvaluationProgressReceiver;
 import com.google.devtools.build.skyframe.EvaluationResult;
+import com.google.devtools.build.skyframe.EventFilter;
 import com.google.devtools.build.skyframe.GraphInconsistencyReceiver;
 import com.google.devtools.build.skyframe.ImmutableDiff;
 import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
@@ -215,6 +217,7 @@ public Builder addExtraSkyFunctions(
       return this;
     }
 
+    @CanIgnoreReturnValue
     public Builder addExtraPrecomputedValues(PrecomputedValue.Injected... extraPrecomputedValues) {
       return this.addExtraPrecomputedValues(Arrays.asList(extraPrecomputedValues));
     }
@@ -422,8 +425,8 @@ private MemoizingEvaluator makeFreshEvaluator() {
         preinjectedDifferencer,
         EvaluationProgressReceiver.NULL,
         GraphInconsistencyReceiver.THROWING,
-        InMemoryMemoizingEvaluator.DEFAULT_STORED_EVENT_FILTER,
-        new MemoizingEvaluator.EmittedEventState(),
+        EventFilter.FULL_STORAGE,
+        new NestedSetVisitor.VisitedState(),
         /*keepEdges=*/ false);
   }
 

src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD

@@ -37,6 +37,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value",
         "//src/main/java/com/google/devtools/build/lib/clock",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/io:file_symlink_cycle_uniqueness_function",

src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java

@@ -31,6 +31,7 @@
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.clock.BlazeClock;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor;
 import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
 import com.google.devtools.build.lib.concurrent.MultiExecutorQueueVisitor;
 import com.google.devtools.build.lib.concurrent.QuiescingExecutor;
@@ -42,7 +43,6 @@
 import com.google.devtools.build.lib.util.GroupedList.GroupedListHelper;
 import com.google.devtools.build.skyframe.EvaluationProgressReceiver.EvaluationState;
 import com.google.devtools.build.skyframe.EvaluationProgressReceiver.NodeState;
-import com.google.devtools.build.skyframe.MemoizingEvaluator.EmittedEventState;
 import com.google.devtools.build.skyframe.NodeEntry.DependencyState;
 import com.google.devtools.build.skyframe.NodeEntry.DirtyState;
 import com.google.devtools.build.skyframe.NodeEntry.DirtyType;
@@ -130,7 +130,7 @@ abstract class AbstractParallelEvaluator {
       Version minimalVersion,
       ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
       ExtendedEventHandler reporter,
-      EmittedEventState emittedEventState,
+      NestedSetVisitor.VisitedState emittedEventState,
       EventFilter storedEventFilter,
       ErrorInfoManager errorInfoManager,
       boolean keepGoing,
@@ -894,9 +894,6 @@ protected void replay(ValueWithMetadata valueWithMetadata) {
     // Replaying actions is done on a small number of nodes, but potentially over a large dependency
     // graph. Under those conditions, using the regular NestedSet flattening with .toList() is more
     // efficient than using NestedSetVisitor's custom traversal logic.
-    evaluatorContext
-        .getReplayingNestedSetPostableVisitor()
-        .visit(valueWithMetadata.getTransitivePostables().toList());
     evaluatorContext
         .getReplayingNestedSetEventVisitor()
         .visit(valueWithMetadata.getTransitiveEvents().toList());

src/main/java/com/google/devtools/build/skyframe/EventFilter.java

@@ -13,24 +13,47 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
-import com.google.devtools.build.lib.events.Event;
-import java.util.function.Predicate;
-
 /** Filters out events which should not be stored during evaluation in {@link ParallelEvaluator}. */
-public interface EventFilter extends Predicate<Event> {
+public interface EventFilter {
+
   /**
-   * Returns true if any events/postables should be stored. Otherwise, optimizations may be made to
-   * avoid doing unnecessary work when evaluating node entries.
+   * Returns true if any {@linkplain com.google.devtools.build.lib.events.Reportable events} should
+   * be stored in skyframe nodes. Otherwise, optimizations may be made to avoid doing unnecessary
+   * work when evaluating node entries.
    */
-  boolean storeEventsAndPosts();
+  boolean storeEvents();
 
   /**
-   * Determines whether stored events and posts should propagate from {@code depKey} to {@code
-   * primaryKey}.
+   * Determines whether stored {@linkplain com.google.devtools.build.lib.events.Reportable events}
+   * should propagate from {@code depKey} to {@code primaryKey}.
    *
-   * <p>Only relevant if {@link #storeEventsAndPosts} returns {@code true}.
+   * <p>Only relevant if {@link #storeEvents} returns {@code true}.
    */
-  default boolean shouldPropagate(SkyKey depKey, SkyKey primaryKey) {
-    return true;
-  }
+  boolean shouldPropagate(SkyKey depKey, SkyKey primaryKey);
+
+  EventFilter FULL_STORAGE =
+      new EventFilter() {
+        @Override
+        public boolean storeEvents() {
+          return true;
+        }
+
+        @Override
+        public boolean shouldPropagate(SkyKey depKey, SkyKey primaryKey) {
+          return true;
+        }
+      };
+
+  EventFilter NO_STORAGE =
+      new EventFilter() {
+        @Override
+        public boolean storeEvents() {
+          return false;
+        }
+
+        @Override
+        public boolean shouldPropagate(SkyKey depKey, SkyKey primaryKey) {
+          throw new UnsupportedOperationException();
+        }
+      };
 }