From 90729a57a865c08fe561be6a49734582becfbb53 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 13 Aug 2018 07:55:35 -0700 Subject: 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 --- .../lib/buildtool/ExecutionProgressReceiver.java | 19 ++++++++------- .../google/devtools/build/lib/events/Reporter.java | 28 ++++++++-------------- .../lib/runtime/ExperimentalEventHandler.java | 11 ++++++++- 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 PROGRESS_MESSAGE_NUMBER_FORMATTER = + ThreadLocal.withInitial( + () -> { + NumberFormat numberFormat = NumberFormat.getIntegerInstance(Locale.ENGLISH); + numberFormat.setGroupingUsed(true); + return numberFormat; + }); // Must be thread-safe! private final Set 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. * *

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 handlers = new ArrayList<>(); + private final ConcurrentLinkedQueue 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. -- cgit v1.2.3