Minor refactoring in java files
In this change:
- mark variables and classes final, so they are
easier to reason about while reading the code
- turn AnalysisUtils into a stateless class, so we
need not to create a throwaway instance of it
- move ImmutableXXX.copyOf calls from
AnalysisResult ctor to call sites, to avoid
making copies if the input is already Immutable
- mark nullable arguments with @Nullable to aid
readability at call sites
- change a map-based duplication check logic from
using "if-contains-then-error-else-put" to
"put-and-fail-if-was-present", because this is
shorter and Map.put returns the old value anyway
- change a LinkedHashMap to HashMap because the
latter requires less memory and we never iterate
over the Map so the linked property is
unnecessary
PiperOrigin-RevId: 286374383
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
index fa7984d..0675ba9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
@@ -44,29 +44,29 @@
AnalysisResult(
BuildConfigurationCollection configurations,
- Collection<ConfiguredTarget> targetsToBuild,
+ ImmutableSet<ConfiguredTarget> targetsToBuild,
ImmutableSet<AspectValue> aspects,
- Collection<ConfiguredTarget> targetsToTest,
- Collection<ConfiguredTarget> targetsToSkip,
+ @Nullable ImmutableList<ConfiguredTarget> targetsToTest,
+ ImmutableSet<ConfiguredTarget> targetsToSkip,
@Nullable String error,
ActionGraph actionGraph,
ArtifactsToOwnerLabels topLevelArtifactsToOwnerLabels,
- Collection<ConfiguredTarget> parallelTests,
- Collection<ConfiguredTarget> exclusiveTests,
+ ImmutableSet<ConfiguredTarget> parallelTests,
+ ImmutableSet<ConfiguredTarget> exclusiveTests,
TopLevelArtifactContext topLevelContext,
PackageRoots packageRoots,
String workspaceName,
Collection<TargetAndConfiguration> topLevelTargetsWithConfigs) {
this.configurations = configurations;
- this.targetsToBuild = ImmutableSet.copyOf(targetsToBuild);
+ this.targetsToBuild = targetsToBuild;
this.aspects = aspects;
- this.targetsToTest = targetsToTest == null ? null : ImmutableList.copyOf(targetsToTest);
- this.targetsToSkip = ImmutableSet.copyOf(targetsToSkip);
+ this.targetsToTest = targetsToTest;
+ this.targetsToSkip = targetsToSkip;
this.error = error;
this.actionGraph = actionGraph;
this.topLevelArtifactsToOwnerLabels = topLevelArtifactsToOwnerLabels;
- this.parallelTests = ImmutableSet.copyOf(parallelTests);
- this.exclusiveTests = ImmutableSet.copyOf(exclusiveTests);
+ this.parallelTests = parallelTests;
+ this.exclusiveTests = exclusiveTests;
this.topLevelContext = topLevelContext;
this.packageRoots = packageRoots;
this.workspaceName = workspaceName;
@@ -100,11 +100,11 @@
}
/**
- * Returns the configured targets to run as tests, or {@code null} if testing was not
- * requested (e.g. "build" command rather than "test" command).
+ * Returns the configured targets to run as tests, or {@code null} if testing was not requested
+ * (e.g. "build" command rather than "test" command).
*/
@Nullable
- public Collection<ConfiguredTarget> getTargetsToTest() {
+ public ImmutableList<ConfiguredTarget> getTargetsToTest() {
return targetsToTest;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
index 677565f..8d7f5e7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
@@ -203,8 +203,7 @@
// We'll get the configs from SkyframeExecutor#getConfigurations, which gets configurations
// for deps including transitions. So to satisfy its API we resolve transitions and repackage
// each target as a Dependency (with a NONE transition if necessary).
- Multimap<BuildConfiguration, Dependency> asDeps =
- AnalysisUtils.targetsToDeps(nodes, ruleClassProvider);
+ Multimap<BuildConfiguration, Dependency> asDeps = targetsToDeps(nodes, ruleClassProvider);
return ConfigurationResolver.getConfigurationsFromExecutor(
nodes, asDeps, eventHandler, skyframeExecutor);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index b3f92ab..f94389e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -542,15 +542,15 @@
};
return new AnalysisResult(
configurations,
- configuredTargets,
+ ImmutableSet.copyOf(configuredTargets),
aspects,
- allTargetsToTest,
- targetsToSkip,
+ allTargetsToTest == null ? null : ImmutableList.copyOf(allTargetsToTest),
+ ImmutableSet.copyOf(targetsToSkip),
error,
actionGraph,
artifactsToOwnerLabelsBuilder.build(),
- parallelTests,
- exclusiveTests,
+ ImmutableSet.copyOf(parallelTests),
+ ImmutableSet.copyOf(exclusiveTests),
topLevelOptions,
skyframeAnalysisResult.getPackageRoots(),
loadingResult.getWorkspaceName(),
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
index 2045933..b695ce0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
@@ -17,7 +17,6 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import java.util.Collection;
import java.util.HashMap;
/**
@@ -35,22 +34,20 @@
private final BuildConfiguration hostConfiguration;
public BuildConfigurationCollection(
- Collection<BuildConfiguration> targetConfigurations,
- BuildConfiguration hostConfiguration)
- throws InvalidConfigurationException {
- this.targetConfigurations = ImmutableList.copyOf(targetConfigurations);
+ ImmutableList<BuildConfiguration> targetConfigurations, BuildConfiguration hostConfiguration)
+ throws InvalidConfigurationException {
+ this.targetConfigurations = targetConfigurations;
this.hostConfiguration = hostConfiguration;
// Except for the host configuration (which may be identical across target configs), the other
// configurations must all have different cache keys or we will end up with problems.
HashMap<String, BuildConfiguration> cacheKeyConflictDetector = new HashMap<>();
for (BuildConfiguration config : targetConfigurations) {
- String cacheKey = config.checksum();
- if (cacheKeyConflictDetector.containsKey(cacheKey)) {
- throw new InvalidConfigurationException("Conflicting configurations: " + config + " & "
- + cacheKeyConflictDetector.get(cacheKey));
+ BuildConfiguration old = cacheKeyConflictDetector.put(config.checksum(), config);
+ if (old != null) {
+ throw new InvalidConfigurationException(
+ "Conflicting configurations: " + config + " & " + old);
}
- cacheKeyConflictDetector.put(cacheKey, config);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index ca6d0d7..3a30216 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -694,7 +694,7 @@
SkyframeExecutor skyframeExecutor)
throws InvalidConfigurationException {
- Map<Label, Target> labelsToTargets = new LinkedHashMap<>();
+ Map<Label, Target> labelsToTargets = new HashMap<>();
for (TargetAndConfiguration targetAndConfig : defaultContext) {
labelsToTargets.put(targetAndConfig.getLabel(), targetAndConfig.getTarget());
}
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
index 30cf557..8da0942 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
@@ -60,14 +60,13 @@
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
- protected CommandEnvironment env;
+ private AnalysisPhaseRunner() {}
- public AnalysisPhaseRunner(CommandEnvironment env) {
- this.env = env;
- }
-
- public AnalysisResult execute(
- BuildRequest request, BuildOptions buildOptions, TargetValidator validator)
+ public static AnalysisResult execute(
+ CommandEnvironment env,
+ BuildRequest request,
+ BuildOptions buildOptions,
+ TargetValidator validator)
throws BuildFailedException, InterruptedException, ViewCreationFailedException,
TargetParsingException, LoadingFailedException, AbruptExitException,
InvalidConfigurationException {
@@ -76,7 +75,7 @@
TargetPatternPhaseValue loadingResult;
Profiler.instance().markPhase(ProfilePhase.LOAD);
try (SilentCloseable c = Profiler.instance().profile("evaluateTargetPatterns")) {
- loadingResult = evaluateTargetPatterns(request, validator);
+ loadingResult = evaluateTargetPatterns(env, request, validator);
}
env.setWorkspaceName(loadingResult.getWorkspaceName());
@@ -121,7 +120,7 @@
try (SilentCloseable c = Profiler.instance().profile("runAnalysisPhase")) {
analysisResult =
- runAnalysisPhase(request, loadingResult, buildOptions, request.getMultiCpus());
+ runAnalysisPhase(env, request, loadingResult, buildOptions, request.getMultiCpus());
}
for (BlazeModule module : env.getRuntime().getBlazeModules()) {
@@ -133,7 +132,7 @@
analysisResult.getAspects());
}
- reportTargets(analysisResult);
+ reportTargets(env, analysisResult);
for (ConfiguredTarget target : analysisResult.getTargetsToSkip()) {
BuildConfiguration config =
@@ -161,8 +160,8 @@
return analysisResult;
}
- private final TargetPatternPhaseValue evaluateTargetPatterns(
- final BuildRequest request, final TargetValidator validator)
+ private static TargetPatternPhaseValue evaluateTargetPatterns(
+ CommandEnvironment env, final BuildRequest request, final TargetValidator validator)
throws LoadingFailedException, TargetParsingException, InterruptedException {
boolean keepGoing = request.getKeepGoing();
TargetPatternPhaseValue result =
@@ -193,7 +192,8 @@
* @throws InterruptedException if the current thread was interrupted.
* @throws ViewCreationFailedException if analysis failed for any reason.
*/
- private AnalysisResult runAnalysisPhase(
+ private static AnalysisResult runAnalysisPhase(
+ CommandEnvironment env,
BuildRequest request,
TargetPatternPhaseValue loadingResult,
BuildOptions targetOptions,
@@ -260,7 +260,7 @@
return analysisResult;
}
- private void reportTargets(AnalysisResult analysisResult) {
+ private static void reportTargets(CommandEnvironment env, AnalysisResult analysisResult) {
Collection<ConfiguredTarget> targetsToBuild = analysisResult.getTargetsToBuild();
Collection<ConfiguredTarget> targetsToTest = analysisResult.getTargetsToTest();
if (targetsToTest != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryBuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryBuildTool.java
index 041ad02..ffdf6ec 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryBuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryBuildTool.java
@@ -47,7 +47,7 @@
import javax.annotation.Nullable;
/** A version of {@link BuildTool} that handles all aquery work. */
-public class AqueryBuildTool extends PostAnalysisQueryBuildTool<ConfiguredTargetValue> {
+public final class AqueryBuildTool extends PostAnalysisQueryBuildTool<ConfiguredTargetValue> {
private final AqueryActionFilter actionFilters;
public AqueryBuildTool(CommandEnvironment env, @Nullable QueryExpression queryExpression)
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index b87fd4f..771d89a 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -66,8 +66,8 @@
private static Logger logger = Logger.getLogger(BuildTool.class.getName());
- protected CommandEnvironment env;
- protected BlazeRuntime runtime;
+ protected final CommandEnvironment env;
+ protected final BlazeRuntime runtime;
/**
* Constructs a BuildTool.
@@ -80,8 +80,7 @@
}
/**
- * The crux of the build system. Builds the targets specified in the request using the specified
- * Executor.
+ * The crux of the build system: builds the targets specified in the request.
*
* <p>Performs loading, analysis and execution for the specified set of targets, honoring the
* configuration options in the BuildRequest. Returns normally iff successful, throws an exception
@@ -139,8 +138,8 @@
initializeOutputFilter(request);
- AnalysisPhaseRunner analysisPhaseRunner = new AnalysisPhaseRunner(env);
- AnalysisResult analysisResult = analysisPhaseRunner.execute(request, buildOptions, validator);
+ AnalysisResult analysisResult =
+ AnalysisPhaseRunner.execute(env, request, buildOptions, validator);
// We cannot move the executionTool down to the execution phase part since it does set up the
// symlinks for tools.
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java
index 23c6898..d9b8863 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java
@@ -25,7 +25,7 @@
import com.google.devtools.build.skyframe.WalkableGraph;
/** A version of {@link BuildTool} that handles all cquery work. */
-public class CqueryBuildTool extends PostAnalysisQueryBuildTool<ConfiguredTarget> {
+public final class CqueryBuildTool extends PostAnalysisQueryBuildTool<ConfiguredTarget> {
public CqueryBuildTool(CommandEnvironment env, QueryExpression queryExpression) {
super(env, queryExpression);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java
index 6c8feb0..374f263 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java
@@ -85,8 +85,9 @@
throws InvalidConfigurationException {
BuildConfiguration hostConfiguration =
skyframeExecutor.getConfiguration(eventHandler, hostConfigurationKey);
- Collection<BuildConfiguration> targetConfigurations =
- skyframeExecutor.getConfigurations(eventHandler, targetConfigurationKeys).values();
+ ImmutableList<BuildConfiguration> targetConfigurations =
+ ImmutableList.copyOf(
+ skyframeExecutor.getConfigurations(eventHandler, targetConfigurationKeys).values());
return new BuildConfigurationCollection(targetConfigurations, hostConfiguration);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index c539012..40fc2ed 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1495,7 +1495,7 @@
configuredTargetProgress.reset();
}
- List<BuildConfiguration> topLevelTargetConfigs =
+ ImmutableList<BuildConfiguration> topLevelTargetConfigs =
getConfigurations(
eventHandler,
PrepareAnalysisPhaseFunction.getTopLevelBuildOptions(buildOptions, multiCpu),
@@ -1907,7 +1907,7 @@
* @throws InvalidConfigurationException if any build options produces an invalid configuration
*/
// TODO(ulfjack): Remove this legacy method after switching to the Skyframe-based implementation.
- private List<BuildConfiguration> getConfigurations(
+ private ImmutableList<BuildConfiguration> getConfigurations(
ExtendedEventHandler eventHandler,
List<BuildOptions> optionsList,
BuildOptions referenceBuildOptions,
