Treat runfiles middlemen as aggregating middlemen.
The goal is to, for an action execution, make only a single skyframe
edge to every required runfiles tree rather than an edge to every
runfiles artifact directly. We accomplish this by pulling the runfiles
artifacts' metadata for a particular runfiles tree through the
appropriate runfiles middlemen artifact in one batch. This CL fixes
https://github.com/bazelbuild/bazel/issues/3217.
This change makes runfiles middlemen more similar to aggregating
middlemen. We however continue to treat runfiles middlemen slightly
differently than true aggregating middlemen. Namely, we do not expand
runfiles middlemen into the final action inputs. The reasons for this
are:
1. It can make latent bugs like
https://github.com/bazelbuild/bazel/issues/4033 more severe.
2. The runfiles tree builder action's output MANIFEST contains
absolute paths, which interferes with remote execution if its metadata
is added to a cache hash.
3. In the sandbox, it causes the runfiles artifacts to be mounted at
their exec path locations in addition to within the runfiles
trees. This makes the sandbox less strict. (Perhaps tolerably?)
4. It saves a modicum of (transient) memory and time to not expand.
If the first two issues are fixed, it could be a nice simplification
to completely replace runfiles middlemen with aggregating middlemen.
Change-Id: I2d2148297f897af4c4c6dc055d87f8a6fad0061e
PiperOrigin-RevId: 198307109
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 5c7d633..f099d76 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -254,8 +254,7 @@
*/
@Nullable
private AllInputs collectInputs(Action action, Environment env) throws InterruptedException {
- Iterable<Artifact> allKnownInputs = Iterables.concat(
- action.getInputs(), action.getRunfilesSupplier().getArtifacts());
+ Iterable<Artifact> allKnownInputs = action.getInputs();
if (action.inputsDiscovered()) {
return new AllInputs(allKnownInputs);
}
@@ -677,11 +676,15 @@
// We have to cache the "digest" of the aggregating value itself,
// because the action cache checker may want it.
inputArtifactData.put(input, aggregatingValue.getSelfData());
- ImmutableList.Builder<Artifact> expansionBuilder = ImmutableList.builder();
- for (Pair<Artifact, FileArtifactValue> pair : aggregatingValue.getInputs()) {
- expansionBuilder.add(pair.first);
+ // Runfiles artifacts are not expanded into the action's inputs but their metadata is
+ // available from the action file cache.
+ if (!(value instanceof RunfilesArtifactValue)) {
+ ImmutableList.Builder<Artifact> expansionBuilder = ImmutableList.builder();
+ for (Pair<Artifact, FileArtifactValue> pair : aggregatingValue.getInputs()) {
+ expansionBuilder.add(pair.first);
+ }
+ expandedArtifacts.put(input, expansionBuilder.build());
}
- expandedArtifacts.put(input, expansionBuilder.build());
} else if (value instanceof TreeArtifactValue) {
TreeArtifactValue treeValue = (TreeArtifactValue) value;
expandedArtifacts.put(input, ImmutableSet.<Artifact>copyOf(treeValue.getChildren()));
@@ -706,7 +709,7 @@
actionFailures++;
// Prefer a catastrophic exception as the one we propagate.
if (firstActionExecutionException == null
- || !firstActionExecutionException.isCatastrophe() && e.isCatastrophe()) {
+ || (!firstActionExecutionException.isCatastrophe() && e.isCatastrophe())) {
firstActionExecutionException = e;
}
rootCauses.addTransitive(e.getRootCauses());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 5cef0fc..1999166 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -290,16 +290,24 @@
}
inputs.add(Pair.of(input, (FileArtifactValue) inputValue));
}
- return new AggregatingArtifactValue(inputs.build(), value);
+ return (action.getActionType() == MiddlemanType.AGGREGATING_MIDDLEMAN)
+ ? new AggregatingArtifactValue(inputs.build(), value)
+ : new RunfilesArtifactValue(inputs.build(), value);
}
/**
* Returns whether this value needs to contain the data of all its inputs. Currently only tests to
- * see if the action is an aggregating middleman action. However, may include runfiles middleman
- * actions and Fileset artifacts in the future.
+ * see if the action is an aggregating or runfiles middleman action. However, may include Fileset
+ * artifacts in the future.
*/
private static boolean isAggregatingValue(ActionAnalysisMetadata action) {
- return action.getActionType() == MiddlemanType.AGGREGATING_MIDDLEMAN;
+ switch (action.getActionType()) {
+ case AGGREGATING_MIDDLEMAN:
+ case RUNFILES_MIDDLEMAN:
+ return true;
+ default:
+ return false;
+ }
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
new file mode 100644
index 0000000..8969a2c
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
@@ -0,0 +1,26 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.util.Pair;
+
+/** The artifacts behind a runfiles middleman. */
+class RunfilesArtifactValue extends AggregatingArtifactValue {
+ RunfilesArtifactValue(
+ ImmutableList<Pair<Artifact, FileArtifactValue>> inputs, FileArtifactValue selfData) {
+ super(inputs, selfData);
+ }
+}
