From bede7b47d88c3892c47382ed39911117e48adc70 Mon Sep 17 00:00:00 2001 From: Janak Ramakrishnan Date: Thu, 17 Nov 2016 18:34:08 +0000 Subject: 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 --- .../devtools/build/lib/analysis/BuildView.java | 48 +++++++++++++++++++++- .../build/lib/pkgcache/LoadingOptions.java | 37 ----------------- .../lib/skyframe/ConfiguredTargetFunction.java | 17 +++++++- .../build/lib/skyframe/SkyframeBuildView.java | 7 +++- .../build/lib/skyframe/SkyframeExecutor.java | 19 +++++---- 5 files changed, 79 insertions(+), 49 deletions(-) (limited to 'src/main/java/com/google/devtools') 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 20b89b0f82..b29d2a2a79 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.Path; 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 @@ public class BuildView { * 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 @@ public class BuildView { 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 @@ public class BuildView { } 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 { + @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 29f05233c6..63938e9677 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 @@ package com.google.devtools.build.lib.pkgcache; 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 @@ import java.util.Set; * 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 @@ public class LoadingOptions extends OptionsBase { 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 { - @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 71a938ab8d..38db025e0b 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; 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 configConditions = getConfigConditions( @@ -256,6 +267,8 @@ final class ConfiguredTargetFunction implements SkyFunction { } 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 21d6f7b12c..a905a9f68a 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 @@ public final class SkyframeBuildView { List values, List aspectKeys, EventBus eventBus, - boolean keepGoing) + boolean keepGoing, + int numThreads) throws InterruptedException, ViewCreationFailedException { enableAnalysis(true); EvaluationResult 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 39090f75a9..4406bd8561 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.Map; 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { Predicate 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 map = ImmutableMap.builder(); @@ -393,8 +397,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { 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 class SkyframeExecutor implements WalkableGraphFactory { public abstract void invalidateTransientErrors(); /** Configures a given set of configured targets. */ - public EvaluationResult configureTargets( + EvaluationResult configureTargets( EventHandler eventHandler, List values, List aspectKeys, - boolean keepGoing) + boolean keepGoing, + int numThreads) throws InterruptedException { checkActive(); @@ -1473,9 +1480,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { 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); } /** -- cgit v1.2.3