Merge CcCompilationContext.getUsedModules() and

CcCompilactionContext.removeDeclaredIncludes() for increased performance (avoid iterating over a large NestedSet twice). Also slightly tweak implementation. While at it, store usedModules as ImmutableList instead of ImmutableSet to conserve some memory. RELNOTES: None. PiperOrigin-RevId: 217863613
luca-digrazia · Sep 4, 2022 · 8e251ca · 8e251ca
1 parent 92f2ca5
commit 8e251ca
Show file tree

Hide file tree

Showing 2 changed files with 3 additions and 50 deletions.
diff --git a/...ild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java b/...ild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java
@@ -22,9 +22,7 @@
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MiddlemanFactory;
-import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.RuleContext;
-import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -33,7 +31,6 @@
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import com.google.devtools.build.lib.skylarkbuildapi.cpp.CcCompilationContextApi;
-import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
 import java.util.Collections;
@@ -114,28 +111,6 @@ public final class CcCompilationContext implements CcCompilationContextApi {
     this.headersCheckingMode = headersCheckingMode;
   }
 
-  @Override
-  public SkylarkNestedSet getSkylarkDefines() {
-    return SkylarkNestedSet.of(
-        String.class, NestedSetBuilder.wrap(Order.STABLE_ORDER, getDefines()));
-  }
-
-  @Override
-  public SkylarkNestedSet getSkylarkHeaders() {
-    return SkylarkNestedSet.of(Artifact.class, getDeclaredIncludeSrcs());
-  }
-
-  @Override
-  public SkylarkNestedSet getSkylarkDeclaredIncludeDirs() {
-    return SkylarkNestedSet.of(
-        String.class,
-        NestedSetBuilder.wrap(
-            Order.STABLE_ORDER,
-            getSystemIncludeDirs().stream()
-                .map(PathFragment::getPathString)
-                .collect(ImmutableList.toImmutableList())));
-  }
-
   /**
    * Returns the transitive compilation prerequisites consolidated into middlemen prerequisites, or
    * an empty set if there are no prerequisites.
@@ -370,23 +345,6 @@ public CppConfiguration.HeadersCheckingMode getHeadersCheckingMode() {
     return headersCheckingMode;
   }
 
-  public static ImmutableList<CcCompilationContext> getCcCompilationContexts(
-      Iterable<? extends TransitiveInfoCollection> deps) {
-    ImmutableList.Builder<CcCompilationContext> ccCompilationContextsBuilder =
-        ImmutableList.builder();
-    for (CcInfo ccInfo : AnalysisUtils.getProviders(deps, CcInfo.PROVIDER)) {
-      ccCompilationContextsBuilder.add(ccInfo.getCcCompilationContext());
-    }
-    return ccCompilationContextsBuilder.build();
-  }
-
-  public static CcCompilationContext merge(Collection<CcCompilationContext> ccCompilationContexts) {
-    CcCompilationContext.Builder builder =
-        new CcCompilationContext.Builder(/* ruleContext= */ null);
-    builder.mergeDependentCcCompilationContexts(ccCompilationContexts);
-    return builder.build();
-  }
-
   /**
    * The parts of the {@code CcCompilationContext} that influence the command line of compilation
    * actions.

diff --git a/...elbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/...elbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -449,16 +449,11 @@ public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionC
       }
     }
 
-    if (usedModules == null) {
-      // There are two paths in which this can be reached:
-      // 1. This is not a modular compilation or one without include scanning. In either case, we
-      //    never compute used modules.
-      // 2. This function has completed on a previous execution, adding all used modules to
-      //    additionalInputs and resetting usedModules to null below.
-      // In either case, there is nothing more to do here.
+    if (!shouldScanIncludes || !shouldPruneModules) {
       return additionalInputs;
     }
 
+    Preconditions.checkState(usedModules != null, "Should have computed used modules");
     Map<Artifact, NestedSet<Artifact>> transitivelyUsedModules =
         computeTransitivelyUsedModules(
             actionExecutionContext.getEnvironmentForDiscoveringInputs(), usedModules);
@@ -470,6 +465,7 @@ public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionC
     // used modules. Combining the NestedSets of transitive deps of the top-level modules also
     // gives us an effective way to compute and store discoveredModules.
     Set<Artifact> topLevel = new LinkedHashSet<>(usedModules);
+    usedModules = null;
     for (NestedSet<Artifact> transitive : transitivelyUsedModules.values()) {
       // It is better to iterate over each nested set here instead of creating a joint one and
       // iterating over it, as this makes use of NestedSet's memoization (each of them has likely
@@ -494,7 +490,6 @@ public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionC
     if (outputFile.isFileType(CppFileTypes.CPP_MODULE)) {
       this.discoveredModules = discoveredModules;
     }
-    usedModules = null;
     return additionalInputs;
   }