Run the analysis phase with as many threads as the user wants. In order to avoid memory blow-up intra-configured-target analysis, use a semaphore to ensure that CPU-bound work only occurs on #CPU-many threads.
RELNOTES: Use --loading_phase_threads to control the number of threads used during the loading/analysis phase.
--
MOS_MIGRATED_REVID=139477645
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 20b89b0..b29d2a2 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
@@ -82,8 +82,10 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.WalkableGraph;
+import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.OptionsParsingException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
@@ -149,6 +151,14 @@
* of a BuildConfiguration.
*/
public static class Options extends OptionsBase {
+ @Option(
+ name = "loading_phase_threads",
+ defaultValue = "-1",
+ category = "what",
+ converter = LoadingPhaseThreadCountConverter.class,
+ help = "Number of parallel threads to use for the loading/analysis phase."
+ )
+ public int loadingPhaseThreads;
@Option(name = "keep_going",
abbrev = 'k',
@@ -502,7 +512,12 @@
try {
skyframeAnalysisResult =
skyframeBuildView.configureTargets(
- eventHandler, topLevelCtKeys, aspectKeys, eventBus, viewOptions.keepGoing);
+ eventHandler,
+ topLevelCtKeys,
+ aspectKeys,
+ eventBus,
+ viewOptions.keepGoing,
+ viewOptions.loadingPhaseThreads);
setArtifactRoots(skyframeAnalysisResult.getPackageRoots());
} finally {
skyframeBuildView.clearInvalidatedConfiguredTargets();
@@ -1107,4 +1122,35 @@
}
return null;
}
+
+ /**
+ * A converter for loading phase thread count. Since the default is not a true constant, we create
+ * a converter here to implement the default logic.
+ */
+ public static final class LoadingPhaseThreadCountConverter implements Converter<Integer> {
+ @Override
+ public Integer convert(String input) throws OptionsParsingException {
+ if ("-1".equals(input)) {
+ // Reduce thread count while running tests. Test cases are typically small, and large thread
+ // pools vying for a relatively small number of CPU cores may induce non-optimal
+ // performance.
+ return System.getenv("TEST_TMPDIR") == null ? 200 : 5;
+ }
+
+ try {
+ int result = Integer.decode(input);
+ if (result < 0) {
+ throw new OptionsParsingException("'" + input + "' must be at least -1");
+ }
+ return result;
+ } catch (NumberFormatException e) {
+ throw new OptionsParsingException("'" + input + "' is not an int");
+ }
+ }
+
+ @Override
+ public String getTypeDescription() {
+ return "an integer";
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java
index 29f0523..63938e9 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java
@@ -15,11 +15,9 @@
import com.google.devtools.build.lib.packages.TestSize;
import com.google.devtools.build.lib.packages.TestTimeout;
-import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionsBase;
-import com.google.devtools.common.options.OptionsParsingException;
import java.util.List;
import java.util.Set;
@@ -27,14 +25,6 @@
* Options that affect how command-line target patterns are resolved to individual targets.
*/
public class LoadingOptions extends OptionsBase {
-
- @Option(name = "loading_phase_threads",
- defaultValue = "-1",
- category = "undocumented",
- converter = LoadingPhaseThreadCountConverter.class,
- help = "Number of parallel threads to use for the loading phase.")
- public int loadingPhaseThreads;
-
@Option(name = "build_tests_only",
defaultValue = "false",
category = "what",
@@ -126,31 +116,4 @@
help = "Use the Skyframe-based target pattern evaluator; implies "
+ "--experimental_interleave_loading_and_analysis.")
public boolean useSkyframeTargetPatternEvaluator;
-
- /**
- * A converter for loading phase thread count. Since the default is not a true constant, we
- * create a converter here to implement the default logic.
- */
- public static final class LoadingPhaseThreadCountConverter implements Converter<Integer> {
- @Override
- public Integer convert(String input) throws OptionsParsingException {
- if ("-1".equals(input)) {
- // Reduce thread count while running tests. Test cases are typically small, and large thread
- // pools vying for a relatively small number of CPU cores may induce non-optimal
- // performance.
- return System.getenv("TEST_TMPDIR") == null ? 200 : 5;
- }
-
- try {
- return Integer.decode(input);
- } catch (NumberFormatException e) {
- throw new OptionsParsingException("'" + input + "' is not an int");
- }
- }
-
- @Override
- public String getTypeDescription() {
- return "an integer";
- }
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index 71a938a..38db025 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -87,6 +87,7 @@
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Set;
+import java.util.concurrent.Semaphore;
import javax.annotation.Nullable;
/**
@@ -131,11 +132,15 @@
private final BuildViewProvider buildViewProvider;
private final RuleClassProvider ruleClassProvider;
+ private final Semaphore cpuBoundSemaphore;
- ConfiguredTargetFunction(BuildViewProvider buildViewProvider,
- RuleClassProvider ruleClassProvider) {
+ ConfiguredTargetFunction(
+ BuildViewProvider buildViewProvider,
+ RuleClassProvider ruleClassProvider,
+ Semaphore cpuBoundSemaphore) {
this.buildViewProvider = buildViewProvider;
this.ruleClassProvider = ruleClassProvider;
+ this.cpuBoundSemaphore = cpuBoundSemaphore;
}
private static boolean useDynamicConfigurations(BuildConfiguration config) {
@@ -195,6 +200,12 @@
TargetAndConfiguration ctgValue = new TargetAndConfiguration(target, configuration);
SkyframeDependencyResolver resolver = view.createDependencyResolver(env);
+
+ // TODO(janakr): this acquire() call may tie up this thread indefinitely, reducing the
+ // parallelism of Skyframe. This is a strict improvement over the prior state of the code, in
+ // which we ran with #processors threads, but ideally we would call #tryAcquire here, and if we
+ // failed, would exit this SkyFunction and restart it when permits were available.
+ cpuBoundSemaphore.acquire();
try {
// Get the configuration targets that trigger this rule's configurable attributes.
ImmutableMap<Label, ConfigMatchingProvider> configConditions = getConfigConditions(
@@ -256,6 +267,8 @@
}
throw new ConfiguredTargetFunctionException(
new ConfiguredValueCreationException(e.getMessage(), analysisRootCause));
+ } finally {
+ cpuBoundSemaphore.release();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 21d6f7b..a905a9f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -203,12 +203,15 @@
List<ConfiguredTargetKey> values,
List<AspectValueKey> aspectKeys,
EventBus eventBus,
- boolean keepGoing)
+ boolean keepGoing,
+ int numThreads)
throws InterruptedException, ViewCreationFailedException {
enableAnalysis(true);
EvaluationResult<ActionLookupValue> result;
try {
- result = skyframeExecutor.configureTargets(eventHandler, values, aspectKeys, keepGoing);
+ result =
+ skyframeExecutor.configureTargets(
+ eventHandler, values, aspectKeys, keepGoing, numThreads);
} finally {
enableAnalysis(false);
}
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 39090f7..4406bd8 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
@@ -152,6 +152,7 @@
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.Callable;
+import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
@@ -339,6 +340,9 @@
Predicate<PathFragment> allowedMissingInputs) {
ConfiguredRuleClassProvider ruleClassProvider =
(ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider();
+ // TODO(janakr): use this semaphore to bound memory usage for SkyFunctions besides
+ // ConfiguredTargetFunction that may have a large temporary memory blow-up.
+ Semaphore cpuBoundSemaphore = new Semaphore(ResourceUsage.getAvailableProcessors());
// We use an immutable map builder for the nice side effect that it throws if a duplicate key
// is inserted.
ImmutableMap.Builder<SkyFunctionName, SkyFunction> map = ImmutableMap.builder();
@@ -393,8 +397,10 @@
map.put(SkyFunctions.TARGET_MARKER, new TargetMarkerFunction());
map.put(SkyFunctions.TRANSITIVE_TARGET, new TransitiveTargetFunction(ruleClassProvider));
map.put(SkyFunctions.TRANSITIVE_TRAVERSAL, new TransitiveTraversalFunction());
- map.put(SkyFunctions.CONFIGURED_TARGET,
- new ConfiguredTargetFunction(new BuildViewProvider(), ruleClassProvider));
+ map.put(
+ SkyFunctions.CONFIGURED_TARGET,
+ new ConfiguredTargetFunction(
+ new BuildViewProvider(), ruleClassProvider, cpuBoundSemaphore));
map.put(SkyFunctions.ASPECT, new AspectFunction(new BuildViewProvider(), ruleClassProvider));
map.put(SkyFunctions.LOAD_SKYLARK_ASPECT, new ToplevelSkylarkAspectFunction());
map.put(SkyFunctions.POST_CONFIGURED_TARGET,
@@ -1461,11 +1467,12 @@
public abstract void invalidateTransientErrors();
/** Configures a given set of configured targets. */
- public EvaluationResult<ActionLookupValue> configureTargets(
+ EvaluationResult<ActionLookupValue> configureTargets(
EventHandler eventHandler,
List<ConfiguredTargetKey> values,
List<AspectValueKey> aspectKeys,
- boolean keepGoing)
+ boolean keepGoing,
+ int numThreads)
throws InterruptedException {
checkActive();
@@ -1473,9 +1480,7 @@
for (AspectValueKey aspectKey : aspectKeys) {
keys.add(aspectKey.getSkyKey());
}
- // Make sure to not run too many analysis threads. This can cause memory thrashing.
- return buildDriver.evaluate(keys, keepGoing, ResourceUsage.getAvailableProcessors(),
- eventHandler);
+ return buildDriver.evaluate(keys, keepGoing, numThreads, eventHandler);
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 7208a22..2463760 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -279,10 +279,10 @@
Set<Flag> flags = config.flags;
LoadingOptions loadingOptions = Options.getDefaults(LoadingOptions.class);
- loadingOptions.loadingPhaseThreads = LOADING_PHASE_THREADS;
BuildView.Options viewOptions = optionsParser.getOptions(BuildView.Options.class);
viewOptions.keepGoing = flags.contains(Flag.KEEP_GOING);
+ viewOptions.loadingPhaseThreads = LOADING_PHASE_THREADS;
BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser);
PackageCacheOptions packageCacheOptions = optionsParser.getOptions(PackageCacheOptions.class);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 3852494..680811c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1524,10 +1524,10 @@
throws Exception {
LoadingOptions loadingOptions = Options.getDefaults(LoadingOptions.class);
- loadingOptions.loadingPhaseThreads = loadingPhaseThreads;
BuildView.Options viewOptions = Options.getDefaults(BuildView.Options.class);
viewOptions.keepGoing = keepGoing;
+ viewOptions.loadingPhaseThreads = loadingPhaseThreads;
LoadingPhaseRunner runner = new LegacyLoadingPhaseRunner(getPackageManager(),
Collections.unmodifiableSet(ruleClassProvider.getRuleClassMap().keySet()));
