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
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 ad42702..c1128f7 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 @@
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 b1cd506..ae66887 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 @@
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 @@
* 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 @@
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 @@
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 @@
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 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 @@
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 8a03ceb..e8683fc 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.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 @@
Collection<Artifact> usedModules = context.getUsedModules(usePic, initialResultSet);
initialResultSet.addAll(usedModules);
initialResult = initialResultSet;
- this.overwrittenVariables = getOverwrittenVariables(usedModules);
}
this.additionalInputs = initialResult;
@@ -537,7 +535,8 @@
@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 @@
@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 @@
// 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 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 8aa4ecc..e6d942b 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 @@
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 @@
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 278e3b0..92a31cde9 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 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
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index 306a73d..355b292 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -686,8 +686,10 @@
getSourceArtifact("module/j.cc"));
assertThat(jObjectAction.getMainIncludeScannerSource()).isEqualTo(
getSourceArtifact("module/j.cc"));
- assertThat(getHeaderModuleFlags(jObjectAction.getCompilerOptions()))
- .containsExactly("b.pic.pcm", "g.pic.pcm");
+ assertThat(getHeaderModules(jObjectAction.getContext().getTransitiveModules(true)))
+ .containsExactly(
+ getBinArtifact("_objs/b/module/b.pic.pcm", "//module:b"),
+ getBinArtifact("_objs/g/module/g.pic.pcm", "//module:g"));
}
@Test
@@ -723,8 +725,6 @@
getGenfilesArtifact("c.cppmap", "//nomodule:e"));
assertThat(getHeaderModules(cObjectAction.getContext().getTransitiveModules(true)))
.containsExactly(getBinArtifact("_objs/b/module/b.pic.pcm", "//module:b"));
- assertThat(getHeaderModuleFlags(cObjectAction.getCompilerOptions()))
- .containsExactly("b.pic.pcm");
getConfiguredTarget("//nomodule:d");
Artifact dObjectArtifact = getBinArtifact("_objs/d/nomodule/d.pic.o", "//nomodule:d");
@@ -738,8 +738,6 @@
getGenfilesArtifact("d.cppmap", "//nomodule:d"));
assertThat(getHeaderModules(dObjectAction.getContext().getTransitiveModules(true)))
.containsExactly(getBinArtifact("_objs/b/module/b.pic.pcm", "//module:b"));
- assertThat(getHeaderModuleFlags(dObjectAction.getCompilerOptions()))
- .containsExactly("b.pic.pcm");
}
@Test
@@ -766,9 +764,9 @@
.containsExactly(
getGenfilesArtifact("b.cppmap", "//module:b"),
getGenfilesArtifact("c.cppmap", "//nomodule:e"));
- assertThat(getHeaderModuleFlags(cObjectAction.getCompilerOptions()))
- .containsExactly("b.pic.pcm");
-
+ assertThat(getHeaderModules(cObjectAction.getContext().getTransitiveModules(true)))
+ .containsExactly(getBinArtifact("_objs/b/module/b.pic.pcm", "//module:b"));
+
getConfiguredTarget("//nomodule:d");
Artifact dObjectArtifact = getBinArtifact("_objs/d/nomodule/d.pic.o", "//nomodule:d");
CppCompileAction dObjectAction = (CppCompileAction) getGeneratingAction(dObjectArtifact);
@@ -776,25 +774,8 @@
.containsExactly(
getGenfilesArtifact("c.cppmap", "//nomodule:c"),
getGenfilesArtifact("d.cppmap", "//nomodule:d"));
- assertThat(getHeaderModuleFlags(dObjectAction.getCompilerOptions()))
- .containsExactly("b.pic.pcm");
- }
-
- @Test
- public void testTopLevelHeaderModules() throws Exception {
- AnalysisMock.get()
- .ccSupport()
- .setupCrosstool(
- mockToolsConfig,
- MockCcSupport.HEADER_MODULES_FEATURE_CONFIGURATION
- + "feature { name: 'header_module_includes_dependencies' }");
- useConfiguration("--cpu=k8");
- setupPackagesForModuleTests(/*useHeaderModules=*/false);
- getConfiguredTarget("//module:j");
- Artifact jObjectArtifact = getBinArtifact("_objs/j/module/j.pic.o", "//module:j");
- CppCompileAction jObjectAction = (CppCompileAction) getGeneratingAction(jObjectArtifact);
- assertThat(getHeaderModuleFlags(jObjectAction.getCompilerOptions()))
- .containsExactly("g.pic.pcm");
+ assertThat(getHeaderModules(dObjectAction.getContext().getTransitiveModules(true)))
+ .containsExactly(getBinArtifact("_objs/b/module/b.pic.pcm", "//module:b"));
}
private void writeSimpleCcLibrary() throws Exception {
