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

5 files changed, 79 insertions, 49 deletions

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<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 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<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 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<Label, ConfigMatchingProvider> 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<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 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<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 @@ 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<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 @@ 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);
   }
 
   /**