From a595c267ab8f4469348a203d2f35d901e4793f21 Mon Sep 17 00:00:00 2001 From: Sara Adams <4410333+saraadams@users.noreply.github.com> Date: Thu, 23 Nov 2023 15:04:19 +0100 Subject: [PATCH] [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 --- .../analyzer/bazelprofile/BazelProfile.java | 15 ++----- .../bazelprofile/BazelProfileConstants.java | 31 +++++++++++-- .../analyzer/bazelprofile/ProfileThread.java | 23 +++++----- .../bazelprofile/BazelProfileTest.java | 43 ++++++++++++++++--- ...arbageCollectionStatsDataProviderTest.java | 4 +- 5 files changed, 83 insertions(+), 33 deletions(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java index b398129..389d491 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java +++ b/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)); } /** diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileConstants.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileConstants.java index 67fbf77..425bc52 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileConstants.java +++ b/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. + * + *

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}. + * + *

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. + * + *

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}. + * + *

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"; diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java index 74be0a4..16efee5 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java +++ b/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 extraMetadata, - List extraEvents, - List completeEvents, - Map> counts, - Map> instants) { - this.threadId = threadId; + @Nullable List extraMetadata, + @Nullable List extraEvents, + @Nullable List completeEvents, + @Nullable Map> counts, + @Nullable Map> 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() { diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java index aa7a018..ccdaeb6 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java +++ b/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(); } } diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java index 717e276..773600d 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java +++ b/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),