Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BazelProfile] More robust GC thread identification #111

Merged
merged 2 commits into from
Nov 23, 2023
Merged

[BazelProfile] More robust GC thread identification #111

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9eb52b3
[BazelProfile] More robust GC thread identification
saraadams Nov 22, 2023
48ae0b7
Address review comment
saraadams Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,22 +172,17 @@ 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()
.filter(event -> event.category.equals(BazelProfileConstants.CAT_GARBAGE_COLLECTION))
.findAny()
.isPresent();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use anyMatch((event) -> ... ) here

}

/**
Expand Down
31 changes: 28 additions & 3 deletions analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
23 changes: 12 additions & 11 deletions analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
43 changes: 37 additions & 6 deletions analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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();
}
}
4 changes: 2 additions & 2 deletions ...gflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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),
Expand Down