Skip to content

Commit

Permalink
Return early instead of nesting.
Browse files Browse the repository at this point in the history
Signed-off-by: Sara Adams <sara.e.adams@gmail.com>
  • Loading branch information
saraadams committed Nov 28, 2023
1 parent 20e04ad commit 985a768
Showing 1 changed file with 63 additions and 57 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -59,69 +58,76 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
try {
boolean remoteCachingUsed =
dataManager.getDatum(RemoteCachingUsed.class).isRemoteCachingUsed();
List<Suggestion> 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<Caveat> 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);
} catch (Throwable t) {
return SuggestionProviderUtil.createSuggestionOutputForFailure(ANALYZER_CLASSNAME, t);
}
}

private static SuggestionOutput noSuggestions() {
return SuggestionProviderUtil.createSuggestionOutput(ANALYZER_CLASSNAME, null, null);
}
}

0 comments on commit 985a768

Please sign in to comment.