From 985a768f99f1b51ba56058e1d8f2f38b92aa65bd Mon Sep 17 00:00:00 2001 From: Sara Adams Date: Tue, 28 Nov 2023 16:04:12 +0100 Subject: [PATCH] Return early instead of nesting. Signed-off-by: Sara Adams --- .../NoCacheActionsSuggestionProvider.java | 120 +++++++++--------- 1 file changed, 63 insertions(+), 57 deletions(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProvider.java index 34883da..96e2055 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProvider.java @@ -15,7 +15,6 @@ package com.engflow.bazel.invocation.analyzer.suggestionproviders; import com.engflow.bazel.invocation.analyzer.Caveat; -import com.engflow.bazel.invocation.analyzer.Suggestion; import com.engflow.bazel.invocation.analyzer.SuggestionCategory; import com.engflow.bazel.invocation.analyzer.SuggestionOutput; import com.engflow.bazel.invocation.analyzer.bazelprofile.BazelEventsUtil; @@ -59,64 +58,67 @@ public SuggestionOutput getSuggestions(DataManager dataManager) { try { boolean remoteCachingUsed = dataManager.getDatum(RemoteCachingUsed.class).isRemoteCachingUsed(); - List suggestions = new ArrayList<>(); + if (!remoteCachingUsed) { + return noSuggestions(); + } + var localActions = dataManager.getDatum(LocalActions.class); + if (localActions.isEmpty()) { + return noSuggestions(); + } + var actionsWithoutRemoteCacheCheck = + localActions.stream() + .filter(action -> !action.hasRemoteCacheCheck()) + .sorted((a, b) -> b.getAction().duration.compareTo(a.getAction().duration)) + .collect(Collectors.toList()); + if (actionsWithoutRemoteCacheCheck.isEmpty()) { + return noSuggestions(); + } List caveats = new ArrayList<>(); - if (remoteCachingUsed) { - var localActions = dataManager.getDatum(LocalActions.class); - if (!localActions.isEmpty()) { - var actionsWithoutRemoteCacheCheck = - localActions.stream() - .filter(action -> !action.hasRemoteCacheCheck()) - .sorted((a, b) -> b.getAction().duration.compareTo(a.getAction().duration)) - .collect(Collectors.toList()); - if (!actionsWithoutRemoteCacheCheck.isEmpty()) { - var targetLabelIncluded = - dataManager.getDatum(FlagValueExperimentalProfileIncludeTargetLabel.class); - if (!targetLabelIncluded.isProfileIncludeTargetLabelEnabled()) { - caveats.add( - SuggestionProviderUtil.createCaveat( - FlagValueExperimentalProfileIncludeTargetLabel.getNotSetButUsefulForStatement( - "investigating actions that are not using remote caching"), - false)); - } - if (actionsWithoutRemoteCacheCheck.size() > maxActions) { - caveats.add( - SuggestionProviderUtil.createCaveat( - String.format( - "Only the %d longest actions that did not check the remote cache of the" - + " %d found were listed.", - maxActions, actionsWithoutRemoteCacheCheck.size()), - true)); - } - StringBuilder recommendation = new StringBuilder(); - recommendation.append( - "Some actions did not check the remote cache. Likely the targets the actions were" - + " executed for include the tag `no-cache` or `no-remote-cache`. Investigate" - + " whether these tags can be removed:"); - actionsWithoutRemoteCacheCheck.stream() - .limit(maxActions) - .forEachOrdered( - action -> - recommendation.append( - "\n" + BazelEventsUtil.summarizeCompleteEvent(action.getAction()))); - suggestions.add( - SuggestionProviderUtil.createSuggestion( - SuggestionCategory.OTHER, - createSuggestionId(SUGGESTION_ID_NO_CACHE_ACTIONS), - "Investigate actions that are not using remote caching", - recommendation.toString(), - null, - ImmutableList.of( - "The profile suggests remote caching was used, but some actions did not" - + " check the remote cache. In most cases, enabling remote caching for" - + " actions improves build performance. An example case where using" - + " remote caching might not be beneficial is when the outputs are very" - + " large and the the cost of downloading them is higher than locally" - + " building the target."), - caveats)); - } - } + var targetLabelIncluded = + dataManager.getDatum(FlagValueExperimentalProfileIncludeTargetLabel.class); + if (!targetLabelIncluded.isProfileIncludeTargetLabelEnabled()) { + caveats.add( + SuggestionProviderUtil.createCaveat( + FlagValueExperimentalProfileIncludeTargetLabel.getNotSetButUsefulForStatement( + "investigating actions that are not using remote caching"), + false)); } + if (actionsWithoutRemoteCacheCheck.size() > maxActions) { + caveats.add( + SuggestionProviderUtil.createCaveat( + String.format( + "Only the %d longest actions that did not check the remote cache of the" + + " %d found were listed.", + maxActions, actionsWithoutRemoteCacheCheck.size()), + true)); + } + StringBuilder recommendation = new StringBuilder(); + recommendation.append( + "Some actions did not check the remote cache. Likely the targets the actions were" + + " executed for include the tag `no-cache` or `no-remote-cache`. Investigate" + + " whether these tags can be removed:"); + actionsWithoutRemoteCacheCheck.stream() + .limit(maxActions) + .forEachOrdered( + action -> + recommendation.append( + "\n" + BazelEventsUtil.summarizeCompleteEvent(action.getAction()))); + var suggestions = + ImmutableList.of( + SuggestionProviderUtil.createSuggestion( + SuggestionCategory.OTHER, + createSuggestionId(SUGGESTION_ID_NO_CACHE_ACTIONS), + "Investigate actions that are not using remote caching", + recommendation.toString(), + null, + ImmutableList.of( + "The profile suggests remote caching was used, but some actions did not" + + " check the remote cache. In most cases, enabling remote caching" + + " for actions improves build performance. An example case where" + + " using remote caching might not be beneficial is when the outputs" + + " are very large and the the cost of downloading them is higher" + + " than locally building the target."), + caveats)); return SuggestionProviderUtil.createSuggestionOutput(ANALYZER_CLASSNAME, suggestions, null); } catch (MissingInputException e) { return SuggestionProviderUtil.createSuggestionOutputForMissingInput(ANALYZER_CLASSNAME, e); @@ -124,4 +126,8 @@ public SuggestionOutput getSuggestions(DataManager dataManager) { return SuggestionProviderUtil.createSuggestionOutputForFailure(ANALYZER_CLASSNAME, t); } } + + private static SuggestionOutput noSuggestions() { + return SuggestionProviderUtil.createSuggestionOutput(ANALYZER_CLASSNAME, null, null); + } }