Skip to content

Commit

Permalink
Remove AggregatingMiddleman from toolchains.
Browse files Browse the repository at this point in the history
After flipping --experimental_enable_aggregating_middleman to false, we can remove the creation & usage of AggregatingMiddleman in the cc toolchain and other parts of the code base that makes use of MiddlemanProvider to add inputs to actions.

The next step is to remove the special handling for AggregatingMiddleman in Skyframe itself.

PiperOrigin-RevId: 381861891
  • Loading branch information
joeleba authored and copybara-github committed Jun 28, 2021
1 parent 46aff0e commit 92ae4e3
Show file tree
Hide file tree
Showing 43 changed files with 38 additions and 822 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -130,34 +130,6 @@ private Pair<Artifact, Action> createMiddleman(
return Pair.of(stampFile, action);
}

/**
* Creates a normal middleman.
*
* <p>If called multiple times, it always returns the same object depending on the {@code
* purpose}. It does not check that the list of inputs is identical. In contrast to other
* middleman methods, this one also returns an object if the list of inputs is empty.
*
* <p>Note: there's no need to synchronize this method; the only use of a field is via a call to
* another synchronized method (getArtifact()).
*/
public Artifact.DerivedArtifact createMiddlemanAllowMultiple(
ActionRegistry registry,
ActionOwner owner,
PathFragment packageDirectory,
String purpose,
NestedSet<Artifact> inputs,
ArtifactRoot middlemanDir) {
String escapedPackageDirectory = Actions.escapedPath(packageDirectory.getPathString());
PathFragment stampName =
PathFragment.create("_middlemen/" + (purpose.startsWith(escapedPackageDirectory)
? purpose : (escapedPackageDirectory + purpose)));
Artifact.DerivedArtifact stampFile =
artifactFactory.getDerivedArtifact(stampName, middlemanDir, actionRegistry.getOwner());
MiddlemanAction.create(
registry, owner, inputs, stampFile, purpose, MiddlemanType.AGGREGATING_MIDDLEMAN);
return stampFile;
}

private Artifact.DerivedArtifact getStampFileArtifact(
String middlemanName, String purpose, ArtifactRoot middlemanDir) {
String escapedFilename = Actions.escapedPath(middlemanName);
Expand Down
29 changes: 0 additions & 29 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ java_library(
":buildinfo/build_info_collection",
":buildinfo/build_info_key",
":common_prerequisite_validator",
":compilation_helper",
":config/auto_cpu_converter",
":config/build_configuration",
":config/build_configuration_option_details",
Expand Down Expand Up @@ -105,7 +104,6 @@ java_library(
":licenses_provider",
":make_environment_event",
":make_variable_supplier",
":middleman_provider",
":no_build_event",
":no_build_request_finished_event",
":options_diff_predicate",
Expand Down Expand Up @@ -332,7 +330,6 @@ java_library(
":label_expander",
":licenses_provider",
":make_variable_supplier",
":middleman_provider",
":options_diff_predicate",
":package_specification_provider",
":platform_options",
Expand Down Expand Up @@ -644,20 +641,6 @@ java_library(
],
)

java_library(
name = "compilation_helper",
srcs = ["CompilationHelper.java"],
deps = [
":analysis_cluster",
":file_provider",
":transitive_info_collection",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//third_party:guava",
],
)

java_library(
name = "configurations_collector",
srcs = [
Expand Down Expand Up @@ -896,18 +879,6 @@ java_library(
],
)

java_library(
name = "middleman_provider",
srcs = ["MiddlemanProvider.java"],
deps = [
":transitive_info_provider",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
],
)

java_library(
name = "no_build_event",
srcs = ["NoBuildEvent.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,36 +174,22 @@ private CommandHelper(

for (Iterable<? extends TransitiveInfoCollection> tools : toolsList) {
for (TransitiveInfoCollection dep : tools) { // (Note: host configuration)
MiddlemanProvider toolMiddleman = dep.getProvider(MiddlemanProvider.class);
if (toolMiddleman != null) {
resolvedToolsBuilder.addTransitive(toolMiddleman.getMiddlemanArtifact());
}

FilesToRunProvider tool = dep.getProvider(FilesToRunProvider.class);
if (tool == null) {
continue;
}

NestedSet<Artifact> files = tool.getFilesToRun();
// It is not obviously correct to skip potentially adding getFilesToRun of the
// FilesToRunProvider. However, for all tools that we know of that provide a middleman, the
// middleman is equivalent to the list of files coming out of getFilesToRun(). Just adding
// all the files creates a substantial performance bottleneck. E.g. a C++ toolchain might
// consist of thousands of files and tracking them one by one for each action that uses them
// is inefficient.
if (toolMiddleman == null) {
resolvedToolsBuilder.addTransitive(files);
}
resolvedToolsBuilder.addTransitive(files);

Label label = AliasProvider.getDependencyLabel(dep);
Artifact executableArtifact = tool.getExecutable();
// If the label has an executable artifact add that to the multimaps.
if (executableArtifact != null) {
mapGet(tempLabelMap, label).add(executableArtifact);
if (toolMiddleman == null) {
// Also send the runfiles when running remotely.
toolsRunfilesBuilder.add(tool.getRunfilesSupplier());
}
// Also send the runfiles when running remotely.
toolsRunfilesBuilder.add(tool.getRunfilesSupplier());
} else {
// Map all depArtifacts to the respective label using the multimaps.
mapGet(tempLabelMap, label).addAll(files.toList());
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -792,10 +792,6 @@ public boolean inprocessSymlinkCreation() {
return options.inprocessSymlinkCreation;
}

public boolean enableAggregatingMiddleman() {
return options.enableAggregatingMiddleman;
}

public boolean skipRunfilesManifests() {
return options.skipRunfilesManifests;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public void finalizeCompileActionBuilder(
.setShouldScanIncludes(false);
} else {
actionBuilder
.addTransitiveMandatoryInputs(toolchain.getAllFilesMiddleman())
.addTransitiveMandatoryInputs(toolchain.getAllFiles())
.setShouldScanIncludes(false);
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,8 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:compilation_helper",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:middleman_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:test/instrumented_files_info",
"//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ private static SpawnAction.Builder singleJarSpawnActionBuilder(RuleContext ruleC
JavaCommon.getHostJavaExecutable(ruleContext),
singleJar,
JavaToolchainProvider.from(ruleContext).getJvmOptions())
.addTransitiveInputs(JavaRuntimeInfo.forHost(ruleContext).javaBaseInputsMiddleman());
.addTransitiveInputs(JavaRuntimeInfo.forHost(ruleContext).javaBaseInputs());
} else {
builder.setExecutable(singleJar);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1774,7 +1774,7 @@ private static SpawnAction.Builder singleJarSpawnActionBuilder(RuleContext ruleC
JavaCommon.getHostJavaExecutable(ruleContext),
singleJar,
JavaToolchainProvider.from(ruleContext).getJvmOptions())
.addTransitiveInputs(JavaRuntimeInfo.forHost(ruleContext).javaBaseInputsMiddleman());
.addTransitiveInputs(JavaRuntimeInfo.forHost(ruleContext).javaBaseInputs());
} else {
builder.setExecutable(singleJar);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ private static void setSingleJarAsExecutable(
JavaCommon.getHostJavaExecutable(ruleContext),
singleJar,
JavaToolchainProvider.from(ruleContext).getJvmOptions())
.addTransitiveInputs(JavaRuntimeInfo.forHost(ruleContext).javaBaseInputsMiddleman());
.addTransitiveInputs(JavaRuntimeInfo.forHost(ruleContext).javaBaseInputs());
} else {
builder.setExecutable(singleJar);
}
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_collection",
"//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_key",
"//src/main/java/com/google/devtools/build/lib/analysis:compilation_helper",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/compilation_mode",
Expand All @@ -65,7 +64,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:licenses_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:make_variable_supplier",
"//src/main/java/com/google/devtools/build/lib/analysis:middleman_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.MiddlemanProvider;
import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
import com.google.devtools.build.lib.analysis.RuleContext;
Expand Down Expand Up @@ -78,7 +77,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
.addNativeDeclaredProvider(toolchain)
.addNativeDeclaredProvider(templateVariableInfo)
.setFilesToBuild(ccToolchainProvider.getAllFiles())
.addProvider(new MiddlemanProvider(ccToolchainProvider.getAllFilesMiddleman()))
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,6 @@ private boolean createDynamicLinkAction(
try {
dynamicLinkActionBuilder.setRuntimeInputs(
ArtifactCategory.DYNAMIC_LIBRARY,
ccToolchain.getDynamicRuntimeLinkMiddleman(ruleErrorConsumer, featureConfiguration),
ccToolchain.getDynamicRuntimeLinkInputs(featureConfiguration));
} catch (EvalException e) {
throw ruleErrorConsumer.throwWithRuleError(e);
Expand All @@ -728,7 +727,6 @@ private boolean createDynamicLinkAction(
try {
dynamicLinkActionBuilder.setRuntimeInputs(
ArtifactCategory.STATIC_LIBRARY,
ccToolchain.getStaticRuntimeLinkMiddleman(ruleErrorConsumer, featureConfiguration),
ccToolchain.getStaticRuntimeLinkInputs(featureConfiguration));
} catch (EvalException e) {
throw ruleErrorConsumer.throwWithRuleError(e);
Expand Down
Loading

0 comments on commit 92ae4e3

Please sign in to comment.