Fix lock contention in Reporter.startTask/.finishTask by:

- Using a thread-safe container to store handlers.
- Using a ThreadLocal NumberFormat to produce readable action counts.
  NumberFormat (or rather the default implementation DecimalFormat) use
  internal state and locking.
- Adding a lock-free fast path for START and FINISH events in
  ExperimentalEventHandler.

RELNOTES: None.
PiperOrigin-RevId: 208479896

3 files changed, 31 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
index 2b614b0630..484298c4c7 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
@@ -46,7 +46,13 @@ public final class ExecutionProgressReceiver
     extends EvaluationProgressReceiver.NullEvaluationProgressReceiver
     implements SkyframeActionExecutor.ProgressSupplier,
         SkyframeActionExecutor.ActionCompletedReceiver {
-  private static final NumberFormat PROGRESS_MESSAGE_NUMBER_FORMATTER;
+  private static final ThreadLocal<NumberFormat> PROGRESS_MESSAGE_NUMBER_FORMATTER =
+      ThreadLocal.withInitial(
+          () -> {
+            NumberFormat numberFormat = NumberFormat.getIntegerInstance(Locale.ENGLISH);
+            numberFormat.setGroupingUsed(true);
+            return numberFormat;
+          });
 
   // Must be thread-safe!
   private final Set<ConfiguredTargetKey> builtTargets;
@@ -59,11 +65,6 @@ public final class ExecutionProgressReceiver
   /** Number of exclusive tests. To be accounted for in progress messages. */
   private final int exclusiveTestsCount;
 
-  static {
-    PROGRESS_MESSAGE_NUMBER_FORMATTER = NumberFormat.getIntegerInstance(Locale.ENGLISH);
-    PROGRESS_MESSAGE_NUMBER_FORMATTER.setGroupingUsed(true);
-  }
-
   /**
    * {@code builtTargets} is accessed through a synchronized set, and so no other access to it is
    * permitted while this receiver is active.
@@ -153,8 +154,10 @@ public final class ExecutionProgressReceiver
   public String getProgressString() {
     return String.format(
         "[%s / %s]",
-        PROGRESS_MESSAGE_NUMBER_FORMATTER.format(completedActions.size()),
-        PROGRESS_MESSAGE_NUMBER_FORMATTER.format(exclusiveTestsCount + enqueuedActions.size()));
+        PROGRESS_MESSAGE_NUMBER_FORMATTER.get().format(completedActions.size()),
+        PROGRESS_MESSAGE_NUMBER_FORMATTER
+            .get()
+            .format(exclusiveTestsCount + enqueuedActions.size()));
   }
 
   ActionExecutionInactivityWatchdog.InactivityMonitor createInactivityMonitor(
diff --git a/src/main/java/com/google/devtools/build/lib/events/Reporter.java b/src/main/java/com/google/devtools/build/lib/events/Reporter.java
index e654748422..d8ea929294 100644
--- a/src/main/java/com/google/devtools/build/lib/events/Reporter.java
+++ b/src/main/java/com/google/devtools/build/lib/events/Reporter.java
@@ -17,8 +17,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.util.io.OutErr;
 import java.io.PrintStream;
-import java.util.ArrayList;
-import java.util.List;
+import java.util.concurrent.ConcurrentLinkedQueue;
 
 /**
  * The reporter is the primary means of reporting events such as errors, warnings, progress
@@ -30,11 +29,11 @@ import java.util.List;
  * reporter's main use is in the blaze runtime and its lifetime is the lifetime of the blaze server.
  *
  * <p>Thread-safe: calls to {@code #report} may be made on any thread. Handlers may be run in an
- * arbitary thread (but right now, they will not be run concurrently).
+ * arbitrary thread (but right now, they will not be run concurrently).
  */
 public final class Reporter implements ExtendedEventHandler, ExceptionListener {
 
-  private final List<EventHandler> handlers = new ArrayList<>();
+  private final ConcurrentLinkedQueue<EventHandler> handlers = new ConcurrentLinkedQueue<>();
   private EventBus eventBus;
 
   /** An OutErr that sends all of its output to this Reporter.
@@ -83,26 +82,19 @@ public final class Reporter implements ExtendedEventHandler, ExceptionListener {
     return outErrToReporter;
   }
 
-  /**
-   * Adds a handler to this reporter.
-   */
-  public synchronized void addHandler(EventHandler handler) {
-    Preconditions.checkNotNull(handler);
-    handlers.add(handler);
+  /** Adds a handler to this reporter. */
+  public void addHandler(EventHandler handler) {
+    handlers.add(Preconditions.checkNotNull(handler));
   }
 
-  /**
-   * Removes handler from this reporter.
-   */
-  public synchronized void removeHandler(EventHandler handler) {
+  /** Removes handler from this reporter. */
+  public void removeHandler(EventHandler handler) {
      handlers.remove(handler);
   }
 
-  /**
-   * This method is called by the build system to report an event.
-   */
+  /** This method is called by the build system to report an event. */
   @Override
-  public synchronized void handle(Event e) {
+  public void handle(Event e) {
     if (e.getKind() != EventKind.ERROR
         && e.getKind() != EventKind.DEBUG
         && e.getTag() != null
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java
index 49ba57ba1b..16caed049b 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java
@@ -312,7 +312,16 @@ public class ExperimentalEventHandler implements EventHandler {
   }
 
   @Override
-  public synchronized void handle(Event event) {
+  public void handle(Event event) {
+    if (!debugAllEvents
+        && !showTimestamp
+        && (event.getKind() == EventKind.START || event.getKind() == EventKind.FINISH)) {
+      return;
+    }
+    handleLocked(event);
+  }
+
+  private synchronized void handleLocked(Event event) {
     try {
       if (debugAllEvents) {
         // Debugging only: show all events visible to the new UI.