Compute module file compile command line flags right before executing the

action. This removes flattening of nested sets (for the transitive/top-level
header modules) in the analysis phase making it about 10% faster. Also remove
the calculation of top-level modules entirely as it doesn't seem to be
necessary and doing it might actually lead to unexpected results when actions
are restored from cache and thus the module input flags are computed from the
actually used inputs (determined from .d files).

--
MOS_MIGRATED_REVID=140738461

5 files changed, 15 insertions, 73 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index ad427022d5..c1128f773a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -85,7 +85,6 @@ public final class CcCommon {
       CppRuleClasses.RANDOM_SEED,
       CppRuleClasses.MODULE_MAPS,
       CppRuleClasses.MODULE_MAP_HOME_CWD,
-      CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES,
       CppRuleClasses.INCLUDE_PATHS,
       CppRuleClasses.PIC,
       CppRuleClasses.PER_OBJECT_DEBUG_INFO,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java
index b1cd506181..ae66887989 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java
@@ -210,10 +210,6 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     return usePic ? picModuleInfo.transitiveModules : moduleInfo.transitiveModules;
   }
 
-  public Set<Artifact> getTopLevelModules(boolean usePic) {
-    return usePic ? picModuleInfo.getTopLevelModules() : moduleInfo.getTopLevelModules();
-  }
-
   public Collection<Artifact> getUsedModules(boolean usePic, Set<Artifact> usedHeaders) {
     return usePic
         ? picModuleInfo.getUsedModules(usedHeaders)
@@ -782,13 +778,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
      * All transitive modules that this context depends on, excluding headerModule.
      */
     private final NestedSet<Artifact> transitiveModules;
-    
-    /**
-     * All implied modules that this context depends on, i.e. all transitiveModules, that are also
-     * a dependency of other transitiveModules.
-     */
-    private final NestedSet<Artifact> impliedModules;
-    
+
     /**
      * All information about mapping transitive headers to transitive modules.
      */
@@ -799,27 +789,14 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
         ImmutableSet<Artifact> modularHeaders,
         ImmutableSet<Artifact> textualHeaders,
         NestedSet<Artifact> transitiveModules,
-        NestedSet<Artifact> impliedModules,
         NestedSet<TransitiveModuleHeaders> transitiveModuleHeaders) {
       this.headerModule = headerModule;
       this.modularHeaders = modularHeaders;
       this.textualHeaders = textualHeaders;
       this.transitiveModules = transitiveModules;
-      this.impliedModules = impliedModules;
       this.transitiveModuleHeaders = transitiveModuleHeaders;
     }
 
-    public Set<Artifact> getTopLevelModules() {
-      Set<Artifact> impliedModules = this.impliedModules.toSet();
-      Set<Artifact> topLevelModules = new LinkedHashSet<>();
-      for (Artifact module : transitiveModules) {
-        if (!impliedModules.contains(module)) {
-          topLevelModules.add(module);
-        }
-      }
-      return topLevelModules;
-    }
-
     public Collection<Artifact> getUsedModules(Set<Artifact> usedHeaders) {
       Set<Artifact> result = new LinkedHashSet<>();
       for (TransitiveModuleHeaders transitiveModule : transitiveModuleHeaders) {
@@ -858,7 +835,6 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
       private final Set<Artifact> modularHeaders = new LinkedHashSet<>();
       private final Set<Artifact> textualHeaders = new LinkedHashSet<>();
       private NestedSetBuilder<Artifact> transitiveModules = NestedSetBuilder.stableOrder();
-      private NestedSetBuilder<Artifact> impliedModules = NestedSetBuilder.stableOrder();
       private NestedSetBuilder<TransitiveModuleHeaders> transitiveModuleHeaders =
           NestedSetBuilder.stableOrder();
 
@@ -889,7 +865,6 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
         modularHeaders.addAll(other.modularHeaders);
         textualHeaders.addAll(other.textualHeaders);
         transitiveModules.addTransitive(other.transitiveModules);
-        impliedModules.addTransitive(other.impliedModules);
         transitiveModuleHeaders.addTransitive(other.transitiveModuleHeaders);
         return this;
       }
@@ -900,12 +875,8 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
       public Builder addTransitive(ModuleInfo moduleInfo) {
         if (moduleInfo.headerModule != null) {
           transitiveModules.add(moduleInfo.headerModule);
-          impliedModules.addTransitive(moduleInfo.transitiveModules);
-        } else {
-          impliedModules.addTransitive(moduleInfo.impliedModules);
         }
         transitiveModules.addTransitive(moduleInfo.transitiveModules);
-        impliedModules.addTransitive(moduleInfo.impliedModules);
         transitiveModuleHeaders.addTransitive(moduleInfo.transitiveModuleHeaders);
         return this;
       }
@@ -922,7 +893,6 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
             modularHeaders,
             ImmutableSet.copyOf(this.textualHeaders),
             transitiveModules,
-            impliedModules.build(),
             transitiveModuleHeaders.build());
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 8a03ceba46..e8683fca58 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -51,7 +51,6 @@ import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
-import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppCompileActionContext.Reply;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
 import com.google.devtools.build.lib.util.DependencySet;
@@ -508,7 +507,6 @@ public class CppCompileAction extends AbstractAction
       Collection<Artifact> usedModules = context.getUsedModules(usePic, initialResultSet);
       initialResultSet.addAll(usedModules);
       initialResult = initialResultSet;
-      this.overwrittenVariables = getOverwrittenVariables(usedModules);
     }
 
     this.additionalInputs = initialResult;
@@ -537,7 +535,8 @@ public class CppCompileAction extends AbstractAction
   @Override
   public Iterable<Artifact> getInputsWhenSkippingInputDiscovery() {
     if (useHeaderModules && cppConfiguration.getSkipUnusedModules()) {
-      return context.getTransitiveModules(usePic);
+      this.additionalInputs = context.getTransitiveModules(usePic).toCollection();
+      return this.additionalInputs;
     }
     return null;
   }
@@ -1040,12 +1039,6 @@ public class CppCompileAction extends AbstractAction
 
   @Override protected void setInputs(Iterable<Artifact> inputs) {
     super.setInputs(inputs);
-    // We need to update overwrittenVariables as those variables might e.g. contain references to
-    // module files that were determined to be unnecessary by input discovery. If we leave them in,
-    // they might lead to unavailable files if e.g. the action is recreated from cache. In addition
-    // to updating the variables here, we also need to update them when they actually change, e.g.
-    // in discoverInputs().
-    this.overwrittenVariables = getOverwrittenVariables(getInputs());
   }
 
   @Override
@@ -1150,11 +1143,7 @@ public class CppCompileAction extends AbstractAction
     // itself is fully determined by the input source files and module maps.
     // A better long-term solution would be to make the compiler to find them automatically and
     // never hand in the .pcm files explicitly on the command line in the first place.
-    Variables overwrittenVariables = this.overwrittenVariables;
-    this.overwrittenVariables = getOverwrittenVariables(ImmutableList.<Artifact>of());
-    // TODO(djasper): Make getArgv() accept a variables parameter.
-    f.addStrings(getArgv());
-    this.overwrittenVariables = overwrittenVariables;
+    f.addStrings(cppCompileCommandLine.getArgv(getInternalOutputFile(), null));
 
     /*
      * getArgv() above captures all changes which affect the compilation
@@ -1180,6 +1169,15 @@ public class CppCompileAction extends AbstractAction
   public void execute(
       ActionExecutionContext actionExecutionContext)
           throws ActionExecutionException, InterruptedException {
+ 
+    if (useHeaderModules) {
+      // If modules pruning is used, modules will be supplied via additionalInputs, otherwise they
+      // are regular inputs.
+      Preconditions.checkNotNull(additionalInputs);
+      this.overwrittenVariables =
+          getOverwrittenVariables(shouldPruneModules ? additionalInputs : getInputs());
+    }
+
     Executor executor = actionExecutionContext.getExecutor();
     CppCompileActionContext.Reply reply;
     try {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 8aa4ecc31c..e6d942bb45 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -350,23 +350,6 @@ public final class CppModel {
     return result.build();
   }
 
-  /**
-   * Select .pcm inputs to pass on the command line depending on whether we are in pic or non-pic
-   * mode.
-   */
-  private ImmutableSet<String> getHeaderModulePaths(
-      CppCompileActionBuilder builder, boolean usePic) {
-    ImmutableSet.Builder<String> result = ImmutableSet.builder();
-    Iterable<Artifact> artifacts =
-        featureConfiguration.isEnabled(CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES)
-            ? builder.getContext().getTopLevelModules(usePic)
-            : builder.getContext().getTransitiveModules(usePic);
-    for (Artifact artifact : artifacts) {
-      result.add(artifact.getExecPathString());
-    }
-    return result.build();
-  }
-
   private void setupCompileBuildVariables(
       CppCompileActionBuilder builder,
       boolean usePic,
@@ -426,8 +409,8 @@ public final class CppModel {
       buildVariables.addCustomBuiltVariable("dependent_module_map_files", sequence);
     }
     if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) {
-      buildVariables.addStringSequenceVariable(
-          "module_files", getHeaderModulePaths(builder, usePic));
+      // Module inputs will be set later when the action is executed.
+      buildVariables.addStringSequenceVariable("module_files", ImmutableSet.<String>of());
     }
     if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
       buildVariables.addStringSequenceVariable(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
index 278e3b0a1e..92a31cde96 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
@@ -224,14 +224,6 @@ public class CppRuleClasses {
   public static final String TRANSITIVE_MODULE_MAPS = "transitive_module_maps";
 
   /**
-   * A string constant for switching on that a header module file includes information about
-   * all its dependencies, so we do not need to pass all transitive dependent header modules on
-   * the command line.
-   */
-  public static final String HEADER_MODULE_INCLUDES_DEPENDENCIES =
-      "header_module_includes_dependencies";
-
-  /**
    * A string constant for the no_legacy_features feature.
    *
    * <p>If this feature is enabled, Bazel will not extend the crosstool configuration with the