From 776b798f279bb28be0f4fb6aeda64205777cb3da Mon Sep 17 00:00:00 2001 From: Sara Adams Date: Wed, 22 Nov 2023 07:15:56 +0100 Subject: [PATCH] [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 --- .../dataproviders/GarbageCollectionStats.java | 30 ++++++++++++++----- .../GarbageCollectionStatsDataProvider.java | 3 +- .../GarbageCollectionSuggestionProvider.java | 14 +++++---- ...arbageCollectionStatsDataProviderTest.java | 15 ++++++---- ...rbageCollectionSuggestionProviderTest.java | 24 +++++++++++++-- 5 files changed, 63 insertions(+), 23 deletions(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStats.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStats.java index bda5b28..b443a69 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStats.java +++ b/analyzer/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 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 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/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProvider.java index 170c5fe..f3109b7 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProvider.java @@ -42,7 +42,8 @@ public GarbageCollectionStats getGarbageCollectionStats() BazelProfile bazelProfile = getDataManager().getDatum(BazelProfile.class); Optional 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/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProvider.java index 21ff105..a21083d 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProvider.java @@ -51,19 +51,21 @@ public SuggestionOutput getSuggestions(DataManager dataManager) { List 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/analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java index 773600d..35d7ef9 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/GarbageCollectionStatsDataProviderTest.java +++ b/analyzer/javatests/com/engflow/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/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProviderTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProviderTest.java index 69a0e38..16f0bbe 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/GarbageCollectionSuggestionProviderTest.java +++ b/analyzer/javatests/com/engflow/bazel/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);