[GarbageCollection] Do not fail on missing GC thread.

Currently, a Bazel profile with no GC threads is identified as invalid. However, in some cases valid Bazel profiles may not include any garbace collection events. With this PR we no longer throw an error if no GC is found, but rather emit a warning that no GC analysis could be performed. Contributes ton #110. Signed-off-by: Sara Adams <[email protected]>
EngFlow · Nov 22, 2023 · 776b798 · 776b798
1 parent 9eb52b3
commit 776b798
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 23 deletions.
diff --git a/...yzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStats.java b/...yzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStats.java
@@ -16,7 +16,11 @@
 
 import com.engflow.bazel.invocation.analyzer.core.Datum;
 import com.engflow.bazel.invocation.analyzer.time.DurationUtil;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import java.time.Duration;
+import java.util.Optional;
+import javax.annotation.Nullable;
 
 /**
  * Data on the garbage collection performed during the invocation. Major garbage collection suspends
@@ -25,28 +29,36 @@
  * an invocation's performance.
  */
 public class GarbageCollectionStats implements Datum {
-  private final Duration majorGarbageCollectionDuration;
+  private final Optional<Duration> majorGarbageCollectionDuration;
+  @Nullable private final String emptyReason;
 
   public GarbageCollectionStats(Duration majorGarbageCollectionDuration) {
-    this.majorGarbageCollectionDuration = majorGarbageCollectionDuration;
+    this.majorGarbageCollectionDuration = Optional.of(majorGarbageCollectionDuration);
+    this.emptyReason = null;
+  }
+
+  public GarbageCollectionStats(String emptyReason) {
+    Preconditions.checkArgument(!Strings.isNullOrEmpty(emptyReason));
+    this.majorGarbageCollectionDuration = Optional.empty();
+    this.emptyReason = emptyReason;
   }
 
   public boolean hasMajorGarbageCollection() {
-    return !majorGarbageCollectionDuration.isZero();
+    return !isEmpty() && !majorGarbageCollectionDuration.get().isZero();
   }
 
-  public Duration getMajorGarbageCollectionDuration() {
+  public Optional<Duration> getMajorGarbageCollectionDuration() {
     return majorGarbageCollectionDuration;
   }
 
   @Override
   public boolean isEmpty() {
-    return false;
+    return majorGarbageCollectionDuration.isEmpty();
   }
 
   @Override
   public String getEmptyReason() {
-    return null;
+    return emptyReason;
   }
 
   @Override
@@ -56,9 +68,13 @@ public String getDescription() {
 
   @Override
   public String getSummary() {
+    if (isEmpty()) {
+      return null;
+    }
     return hasMajorGarbageCollection()
         ? String.format(
-            "Major GC duration of %s", DurationUtil.formatDuration(majorGarbageCollectionDuration))
+            "Major GC duration of %s",
+            DurationUtil.formatDuration(majorGarbageCollectionDuration.get()))
         : "No major GC occurred";
   }
 }
diff --git a/...m/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProvider.java b/...m/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProvider.java
@@ -42,7 +42,8 @@ public GarbageCollectionStats getGarbageCollectionStats()
     BazelProfile bazelProfile = getDataManager().getDatum(BazelProfile.class);
     Optional<ProfileThread> garbageCollectorThread = bazelProfile.getGarbageCollectorThread();
     if (garbageCollectorThread.isEmpty()) {
-      throw new InvalidProfileException("Unable to find garbage collector thread.");
+      return new GarbageCollectionStats(
+          "Failed to find a garbage collector thread in the Bazel profile.");
     }
     Duration majorGarbageCollection =
         garbageCollectorThread.get().getCompleteEvents().stream()

diff --git a/...ow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProvider.java b/...ow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProvider.java
@@ -51,19 +51,21 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
       List<Suggestion> suggestions = new ArrayList<>();
       GarbageCollectionStats gcStats = dataManager.getDatum(GarbageCollectionStats.class);
 
+      if (gcStats.isEmpty()) {
+        return SuggestionProviderUtil.createSuggestionOutputForEmptyInput(
+            ANALYZER_CLASSNAME, EMPTY_REASON_PREFIX + gcStats.getEmptyReason());
+      }
       if (gcStats.hasMajorGarbageCollection()) {
+        Duration majorGcDuration = gcStats.getMajorGarbageCollectionDuration().get();
         TotalDuration totalDurationDatum = dataManager.getDatum(TotalDuration.class);
         if (totalDurationDatum.isEmpty()) {
           return SuggestionProviderUtil.createSuggestionOutputForEmptyInput(
               ANALYZER_CLASSNAME, EMPTY_REASON_PREFIX + totalDurationDatum.getEmptyReason());
         }
         Duration totalDuration = totalDurationDatum.getTotalDuration().get();
-        double percentOfTotal =
-            DurationUtil.getPercentageOf(
-                gcStats.getMajorGarbageCollectionDuration(), totalDuration);
+        double percentOfTotal = DurationUtil.getPercentageOf(majorGcDuration, totalDuration);
         if (percentOfTotal >= MAJOR_GC_MIN_PERCENTAGE) {
-          Duration optimalDuration =
-              totalDuration.minus(gcStats.getMajorGarbageCollectionDuration());
+          Duration optimalDuration = totalDuration.minus(majorGcDuration);
           PotentialImprovement potentialImprovement =
               SuggestionProviderUtil.createPotentialImprovement(
                   String.format(
@@ -78,7 +80,7 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
                   Locale.US,
                   "%s or %.2f%% of the invocation is spent on major garbage collection which"
                       + " suspends all other threads.",
-                  DurationUtil.formatDuration(gcStats.getMajorGarbageCollectionDuration()),
+                  DurationUtil.formatDuration(majorGcDuration),
                   percentOfTotal);
 
           String titleIncreaseHeapSize = "Increase the Java heap size available to Bazel";

diff --git a/...gflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java b/...gflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java
@@ -22,11 +22,9 @@
 import static com.engflow.bazel.invocation.analyzer.WriteBazelProfile.thread;
 import static com.engflow.bazel.invocation.analyzer.WriteBazelProfile.trace;
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.assertThrows;
 
 import com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants;
 import com.engflow.bazel.invocation.analyzer.core.DuplicateProviderException;
-import com.engflow.bazel.invocation.analyzer.core.InvalidProfileException;
 import com.engflow.bazel.invocation.analyzer.time.TimeUtil;
 import com.engflow.bazel.invocation.analyzer.time.Timestamp;
 import java.time.Duration;
@@ -66,8 +64,9 @@ public void shouldReturnMajorGarbageCollection() throws Exception {
                                 singleGcDuration))))));
 
     GarbageCollectionStats gcStats = provider.getGarbageCollectionStats();
+    assertThat(gcStats.isEmpty()).isFalse();
     assertThat(gcStats.hasMajorGarbageCollection()).isTrue();
-    assertThat(gcStats.getMajorGarbageCollectionDuration())
+    assertThat(gcStats.getMajorGarbageCollectionDuration().get())
         .isEqualTo(singleGcDuration.multipliedBy(4));
   }
 
@@ -93,14 +92,18 @@ public void shouldReturnNoMajorGarbageCollection() throws Exception {
                                 singleGcDuration))))));
 
     GarbageCollectionStats gcStats = provider.getGarbageCollectionStats();
+    assertThat(gcStats.isEmpty()).isFalse();
     assertThat(gcStats.hasMajorGarbageCollection()).isFalse();
-    assertThat(gcStats.getMajorGarbageCollectionDuration()).isEqualTo(Duration.ZERO);
+    assertThat(gcStats.getMajorGarbageCollectionDuration().get()).isEqualTo(Duration.ZERO);
   }
 
   @Test
-  public void shouldThrowWhenNoMajorGarbageCollectorThreadIsPresent() throws Exception {
+  public void shouldReturnEmptyGarbageCollectionStatsWhenNoGarbageCollectorThreadIsPresent()
+      throws Exception {
     useProfile(metaData(), trace(mainThread()));
 
-    assertThrows(InvalidProfileException.class, () -> provider.getGarbageCollectionStats());
+    GarbageCollectionStats gcStats = provider.getGarbageCollectionStats();
+    assertThat(gcStats.isEmpty()).isTrue();
+    assertThat(gcStats.hasMajorGarbageCollection()).isFalse();
   }
 }
diff --git a/...azel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProviderTest.java b/...azel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProviderTest.java
@@ -49,9 +49,27 @@ public void setup() throws Exception {
     suggestionProvider = new GarbageCollectionSuggestionProvider();
   }
 
+  @Test
+  public void shouldNotReturnSuggestionForEmptyGarbageCollectionStats() {
+    String emptyReason = "The GC stats are empty!";
+    garbageCollectionStats = new GarbageCollectionStats(emptyReason);
+
+    SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager);
+
+    assertThat(suggestionOutput.getAnalyzerClassname())
+        .isEqualTo(GarbageCollectionSuggestionProvider.class.getName());
+    assertThat(suggestionOutput.getSuggestionList()).isEmpty();
+    assertThat(suggestionOutput.hasFailure()).isFalse();
+    assertThat(suggestionOutput.getCaveatList()).hasSize(1);
+    assertThat(suggestionOutput.getCaveat(0).getMessage())
+        .contains(GarbageCollectionSuggestionProvider.EMPTY_REASON_PREFIX);
+    assertThat(suggestionOutput.getCaveat(0).getMessage()).contains(emptyReason);
+  }
+
   @Test
   public void shouldNotReturnSuggestionForEmptyTotalDuration() {
-    totalDuration = new TotalDuration("empty");
+    String emptyReason = "The total duration is empty!";
+    totalDuration = new TotalDuration(emptyReason);
 
     SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager);
 
@@ -62,7 +80,7 @@ public void shouldNotReturnSuggestionForEmptyTotalDuration() {
     assertThat(suggestionOutput.getCaveatList()).hasSize(1);
     assertThat(suggestionOutput.getCaveat(0).getMessage())
         .contains(GarbageCollectionSuggestionProvider.EMPTY_REASON_PREFIX);
-    assertThat(suggestionOutput.getCaveat(0).getMessage()).contains("empty");
+    assertThat(suggestionOutput.getCaveat(0).getMessage()).contains(emptyReason);
   }
 
   @Test
@@ -101,7 +119,7 @@ public void shouldReturnSuggestionsForMajorGarbageCollection() {
                 Locale.US,
                 "%s or %.2f%% of the invocation is spent on major garbage collection",
                 DurationUtil.formatDuration(
-                    garbageCollectionStats.getMajorGarbageCollectionDuration()),
+                    garbageCollectionStats.getMajorGarbageCollectionDuration().get()),
                 percent));
 
     Suggestion rules = suggestionOutput.getSuggestionList().get(1);