[7.6.0] Performance improvements for builds with many top-level targe…

…ts (#25492) * 3e115b9 * e12fdd9 --------- Co-authored-by: Googler <jhorvitz@google.com>
bazelbuild · Mar 11, 2025 · 519b5fd · 519b5fd
1 parent b46d382
commit 519b5fd
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 18 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
@@ -13,10 +13,11 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -63,6 +64,7 @@
 import com.google.devtools.build.lib.packages.NativeAspectClass;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
+import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.StarlarkAspectClass;
 import com.google.devtools.build.lib.packages.Target;
@@ -166,7 +168,7 @@ public BuildView(
     this.directories = directories;
     this.coverageReportActionFactory = coverageReportActionFactory;
     this.ruleClassProvider = ruleClassProvider;
-    this.skyframeExecutor = Preconditions.checkNotNull(skyframeExecutor);
+    this.skyframeExecutor = checkNotNull(skyframeExecutor);
     this.skyframeBuildView = skyframeExecutor.getSkyframeBuildView();
   }
 
@@ -232,13 +234,7 @@ public AnalysisResult update(
     skyframeBuildView.resetProgressReceiver();
     skyframeExecutor.setBaselineConfiguration(targetOptions);
 
-    ImmutableMap.Builder<Label, Target> labelToTargetsMapBuilder =
-        ImmutableMap.builderWithExpectedSize(loadingResult.getTargetLabels().size());
-    loadingResult
-        .getTargets(eventHandler, skyframeExecutor.getPackageManager())
-        .forEach(target -> labelToTargetsMapBuilder.put(target.getLabel(), target));
-    ImmutableMap<Label, Target> labelToTargetMap = labelToTargetsMapBuilder.buildOrThrow();
-
+    ImmutableMap<Label, Target> labelToTargetMap = constructLabelToTargetMap(loadingResult);
     eventBus.post(new AnalysisPhaseStartedEvent(labelToTargetMap.values()));
 
     // Prepare the analysis phase
@@ -291,7 +287,7 @@ public AnalysisResult update(
             (RuleContextConstraintSemantics) ruleClassProvider.getConstraintSemantics());
         // We wait until now to setup for execution, in case the artifact factory was reset
         // due to a config change.
-        Preconditions.checkNotNull(executionSetupCallback).prepareForExecution();
+        checkNotNull(executionSetupCallback).prepareForExecution();
         skyframeAnalysisResult =
             skyframeBuildView.analyzeAndExecuteTargets(
                 eventHandler,
@@ -303,8 +299,8 @@ public AnalysisResult update(
                 explicitTargetPatterns,
                 eventBus,
                 bugReporter,
-                Preconditions.checkNotNull(resourceManager), // non-null for skymeld.
-                Preconditions.checkNotNull(buildResultListener), // non-null for skymeld.
+                checkNotNull(resourceManager), // non-null for skymeld.
+                checkNotNull(buildResultListener), // non-null for skymeld.
                 (configuredTargets, allTargetsToTest) ->
                     memoizedGetCoverageArtifactsHelper(
                         configuredTargets, allTargetsToTest, eventHandler, eventBus, loadingResult),
@@ -414,6 +410,20 @@ public AnalysisResult update(
     return result;
   }
 
+  private ImmutableMap<Label, Target> constructLabelToTargetMap(
+      TargetPatternPhaseValue loadingResult) throws InterruptedException {
+    ImmutableSet<Label> labels = loadingResult.getTargetLabels();
+    ImmutableMap.Builder<Label, Target> builder =
+        ImmutableMap.builderWithExpectedSize(labels.size());
+    for (Label label : labels) {
+      Package pkg =
+          checkNotNull(skyframeExecutor.getExistingPackage(label.getPackageIdentifier()), label);
+      Target target = checkNotNull(pkg.getTargets().get(label.getName()), label);
+      builder.put(label, target);
+    }
+    return builder.buildOrThrow();
+  }
+
   private ImmutableList<TopLevelAspectsKey> createTopLevelAspectKeys(
       List<String> aspects,
       ImmutableMap<String, String> aspectsParameters,
@@ -533,7 +543,7 @@ private AnalysisResult createResult(
     // build-info and build-changelist.
     ImmutableList<Artifact> buildInfoArtifacts =
         skyframeExecutor.getWorkspaceStatusArtifacts(eventHandler);
-    Preconditions.checkState(buildInfoArtifacts.size() == 2, buildInfoArtifacts);
+    checkState(buildInfoArtifacts.size() == 2, buildInfoArtifacts);
 
     // Extra actions
     addExtraActionsIfRequested(

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
@@ -2913,13 +2913,14 @@ public void evaluated(
             skyKey, newValue, newError, evaluationSuccessState, state, directDeps);
       }
 
-      // After a PACKAGE node is evaluated, all targets and the labels associated with this package
-      // should have been added to the InMemoryGraph. So it is safe to remove relevant labels from
-      // weak interner.
+      // After a PACKAGE node is freshly computed, all targets and the labels associated with this
+      // package should have been added to the InMemoryGraph. So it is safe to remove relevant
+      // labels from weak interner.
       LabelInterner labelInterner = Label.getLabelInterner();
       if (labelInterner.enabled()
           && skyKey.functionName().equals(SkyFunctions.PACKAGE)
-          && newValue != null) {
+          && newValue != null
+          && directDeps != null) {
         checkState(newValue instanceof PackageValue, newValue);
 
         Package pkg = ((PackageValue) newValue).getPackage();
@@ -3654,7 +3655,7 @@ default ThreadStateReceiver makeThreadStateReceiver(SkyKey key) {
   }
 
   @Nullable
-  private Package getExistingPackage(PackageIdentifier id) throws InterruptedException {
+  public Package getExistingPackage(PackageIdentifier id) throws InterruptedException {
     var value = (PackageValue) memoizingEvaluator.getExistingValue(id);
     if (value == null) {
       return null;