[BazelProfile] More robust GC thread identification (#111)

The thread in which GC events are listed has been renamed multiple
times, so the code for extracting the right thread by name is
error-prone.
Instead of testing the name, search for the GC thread by its included
events. This assumes that there is only one thread that contains GC
events.

Contributes to #110

---------

Signed-off-by: Sara Adams <[email protected]>

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java

@@ -172,22 +172,15 @@ static boolean isMainThread(ProfileThread thread) {
   }
 
   /**
-   * Returns whether the passed-in thread looks like the garbage collection, both for newer and
-   * older versions of Bazel. See
-   * https://github.com/bazelbuild/bazel/commit/a03674e6297ed5f6f740889cba8780d7c4ffe05c for when
-   * the naming of the garbage collection thread was changed.
+   * Returns whether the passed-in thread has at least one garbage collection event.
    *
    * @param thread the thread to check
-   * @return whether the thread looks like it is the garbage collection thread
+   * @return whether the thread includes a garbage collection event
    */
   @VisibleForTesting
   static boolean isGarbageCollectorThread(ProfileThread thread) {
-    String name = thread.getName();
-    if (Strings.isNullOrEmpty(name)) {
-      return false;
-    }
-    return name.equals(BazelProfileConstants.THREAD_GARBAGE_COLLECTOR)
-        || name.equals(BazelProfileConstants.THREAD_GARBAGE_COLLECTOR_OLD);
+    return thread.getCompleteEvents().stream()
+        .anyMatch(event -> event.category.equals(BazelProfileConstants.CAT_GARBAGE_COLLECTION));
   }
 
   /**

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileConstants.java

@@ -21,9 +21,34 @@ public class BazelProfileConstants {
   // Thread names: event.name == METADATA_THREAD_NAME && event.args.name == constant below
   // These constants should not be used outside this package.
   @VisibleForTesting public static final String THREAD_CRITICAL_PATH = "Critical Path";
-  @VisibleForTesting public static final String THREAD_GARBAGE_COLLECTOR = "Garbage Collector";
-  // See https://github.com/bazelbuild/bazel/commit/a03674e6297ed5f6f740889cba8780d7c4ffe05c
-  static final String THREAD_GARBAGE_COLLECTOR_OLD = "Service Thread";
+
+  /**
+   * Deprecated.
+   *
+   * <p>The thread that contains GC events has been renamed multiple times, so it's discouraged to
+   * use it to filter for GC events. Instead, match the event category against {@link
+   * #CAT_GARBAGE_COLLECTION}.
+   *
+   * <p>Used thread names over time include "Service Thread", "Garbage Collector" and "Notification
+   * Thread". E.g. see
+   * https://github.com/bazelbuild/bazel/commit/a03674e6297ed5f6f740889cba8780d7c4ffe05c
+   */
+  @Deprecated @VisibleForTesting
+  public static final String THREAD_GARBAGE_COLLECTOR = "Garbage Collector";
+
+  /**
+   * Deprecated.
+   *
+   * <p>The thread that contains GC events has been renamed multiple times, so it's discouraged to
+   * use it to filter for GC events. Instead, match the event category against {@link
+   * #CAT_GARBAGE_COLLECTION}.
+   *
+   * <p>Used thread names over time include "Service Thread", "Garbage Collector" and "Notification
+   * Thread". E.g. see
+   * https://github.com/bazelbuild/bazel/commit/a03674e6297ed5f6f740889cba8780d7c4ffe05c
+   */
+  @Deprecated static final String THREAD_GARBAGE_COLLECTOR_OLD = "Service Thread";
+
   @VisibleForTesting public static final String THREAD_MAIN = "Main Thread";
   // See https://github.com/bazelbuild/bazel/commit/a03674e6297ed5f6f740889cba8780d7c4ffe05c
   static final String THREAD_MAIN_OLD_PREFIX = "grpc-command";

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java

@@ -19,6 +19,7 @@
 import com.engflow.bazel.invocation.analyzer.traceeventformat.InstantEvent;
 import com.engflow.bazel.invocation.analyzer.traceeventformat.TraceEventFormatConstants;
 import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterators;
@@ -78,19 +79,19 @@ public ProfileThread(
       ThreadId threadId,
       @Nullable String name,
       @Nullable Integer sortIndex,
-      List<JsonObject> extraMetadata,
-      List<JsonObject> extraEvents,
-      List<CompleteEvent> completeEvents,
-      Map<String, List<CounterEvent>> counts,
-      Map<String, List<InstantEvent>> instants) {
-    this.threadId = threadId;
+      @Nullable List<JsonObject> extraMetadata,
+      @Nullable List<JsonObject> extraEvents,
+      @Nullable List<CompleteEvent> completeEvents,
+      @Nullable Map<String, List<CounterEvent>> counts,
+      @Nullable Map<String, List<InstantEvent>> instants) {
+    this.threadId = Preconditions.checkNotNull(threadId);
     this.name = name;
     this.sortIndex = sortIndex;
-    this.extraMetadata = extraMetadata;
-    this.extraEvents = extraEvents;
-    this.completeEvents = completeEvents;
-    this.counts = counts;
-    this.instants = instants;
+    this.extraMetadata = extraMetadata == null ? new ArrayList<>() : extraMetadata;
+    this.extraEvents = extraEvents == null ? new ArrayList<>() : extraEvents;
+    this.completeEvents = completeEvents == null ? new ArrayList<>() : completeEvents;
+    this.counts = counts == null ? new HashMap<>() : counts;
+    this.instants = instants == null ? new HashMap<>() : instants;
   }
 
   public ThreadId getThreadId() {

analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java

@@ -371,7 +371,7 @@ public void isMainThreadShouldReturnTrueOnOldName() {
   }
 
   @Test
-  public void isGarbageCollectorThreadShouldReturnFalseOnUnnamedProfileThread() {
+  public void isGarbageCollectorThreadShouldReturnFalseOnEmptyProfileThread() {
     ProfileThread thread = new ProfileThread(new ThreadId(0, 0));
     assertThat(BazelProfile.isGarbageCollectorThread(thread)).isFalse();
   }
@@ -385,17 +385,48 @@ public void isGarbageCollectorShouldReturnFalseOnOtherProfileThread() {
   }
 
   @Test
-  public void isGarbageCollectorShouldReturnTrueOnNewName() {
+  public void isGarbageCollectorShouldReturnFalseOnGCThreadWithoutGCEvents() {
     ProfileThread thread =
         new ProfileThread(
-            new ThreadId(0, 0), "Garbage Collector", null, null, null, null, null, null);
-    assertThat(BazelProfile.isGarbageCollectorThread(thread)).isTrue();
+            new ThreadId(0, 0),
+            "Garbage Collector",
+            null,
+            null,
+            null,
+            Lists.newArrayList(
+                new CompleteEvent(
+                    null,
+                    BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT,
+                    Timestamp.ofMicros(11),
+                    TimeUtil.getDurationForMicros(10),
+                    1,
+                    1,
+                    ImmutableMap.of())),
+            null,
+            null);
+    assertThat(BazelProfile.isGarbageCollectorThread(thread)).isFalse();
   }
 
   @Test
-  public void isGarbageCollectorShouldReturnTrueOnOldName() {
+  public void isGarbageCollectorShouldReturnTrueOnThreadWithGC() {
     ProfileThread thread =
-        new ProfileThread(new ThreadId(0, 0), "Service Thread", null, null, null, null, null, null);
+        new ProfileThread(
+            new ThreadId(0, 0),
+            "Foo Thread",
+            null,
+            null,
+            null,
+            Lists.newArrayList(
+                new CompleteEvent(
+                    null,
+                    BazelProfileConstants.CAT_GARBAGE_COLLECTION,
+                    Timestamp.ofMicros(11),
+                    TimeUtil.getDurationForMicros(10),
+                    1,
+                    1,
+                    ImmutableMap.of())),
+            null,
+            null);
     assertThat(BazelProfile.isGarbageCollectorThread(thread)).isTrue();
   }
 }

analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java

@@ -54,7 +54,7 @@ public void shouldReturnMajorGarbageCollection() throws Exception {
             thread(
                 0,
                 0,
-                BazelProfileConstants.THREAD_GARBAGE_COLLECTOR,
+                "Thread with major GC",
                 concat(
                     sequence(
                         Stream.of(50_000, 100_000, 150_000, 200_000),
@@ -81,7 +81,7 @@ public void shouldReturnNoMajorGarbageCollection() throws Exception {
             thread(
                 0,
                 0,
-                BazelProfileConstants.THREAD_GARBAGE_COLLECTOR,
+                "Thread without major GC",
                 concat(
                     sequence(
                         Stream.of(50_000, 100_000, 150_000, 200_000),