Remove special handling of middleman actions.
Now that the only type of middleman is the runfiles one, we don't need to jump through a lot of hoops we had to for other sorts of middlemen that did not represent actual inputs to actions.
MiddlemanType is kept for the time being for an easier rollback of this change, if need be.
RELNOTES: Actions that create runfiles trees are now considered regular actions. This means that they are now reported in statistics, critical paths and the like.
PiperOrigin-RevId: 690642037
Change-Id: I472df8a8ad8235e5c8f616c8e354a5fd4739630e
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index e765121..081e977 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -42,7 +42,6 @@
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
import com.google.devtools.build.lib.vfs.OutputPermissions;
-import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.FileNotFoundException;
import java.io.IOException;
@@ -231,8 +230,8 @@
}
}
for (Artifact artifact : actionInputs.toList()) {
- mdMap.put(
- artifact.getExecPathString(), getInputMetadataMaybe(inputMetadataProvider, artifact));
+ FileArtifactValue inputMetadata = getInputMetadataMaybe(inputMetadataProvider, artifact);
+ mdMap.put(artifact.getExecPathString(), inputMetadata);
}
return !Arrays.equals(MetadataDigestUtils.fromMetadata(mdMap), entry.getFileDigest());
}
@@ -606,6 +605,7 @@
actionCache.accountMiss(MissReason.UNCONDITIONAL_EXECUTION);
return true;
}
+
if (entry == null) {
reportNewAction(handler, action);
actionCache.accountMiss(MissReason.NOT_CACHED);
@@ -657,7 +657,7 @@
InputMetadataProvider inputMetadataProvider, Artifact artifact) throws IOException {
FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(artifact);
return (metadata != null && artifact.isConstantMetadata())
- ? ConstantMetadataValue.INSTANCE
+ ? FileArtifactValue.ConstantMetadataValue.INSTANCE
: metadata;
}
@@ -666,7 +666,7 @@
throws IOException, InterruptedException {
FileArtifactValue metadata = outputMetadataStore.getOutputMetadata(artifact);
return (metadata != null && artifact.isConstantMetadata())
- ? ConstantMetadataValue.INSTANCE
+ ? FileArtifactValue.ConstantMetadataValue.INSTANCE
: metadata;
}
@@ -992,44 +992,4 @@
}
}
- private static final class ConstantMetadataValue extends FileArtifactValue
- implements FileArtifactValue.Singleton {
- static final ConstantMetadataValue INSTANCE = new ConstantMetadataValue();
- // This needs to not be of length 0, so it is distinguishable from a missing digest when written
- // into a Fingerprint.
- private static final byte[] DIGEST = new byte[1];
-
- private ConstantMetadataValue() {}
-
- @Override
- public FileStateType getType() {
- return FileStateType.REGULAR_FILE;
- }
-
- @Override
- public byte[] getDigest() {
- return DIGEST;
- }
-
- @Override
- public FileContentsProxy getContentsProxy() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public long getSize() {
- return 0;
- }
-
- @Override
- public long getModifiedTime() {
- return -1;
- }
-
- @Override
- public boolean wasModifiedSinceDigest(Path path) {
- throw new UnsupportedOperationException(
- "ConstantMetadataValue doesn't support wasModifiedSinceDigest " + path.toString());
- }
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionMiddlemanEvent.java b/src/main/java/com/google/devtools/build/lib/actions/ActionMiddlemanEvent.java
deleted file mode 100644
index 767a30c..0000000
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionMiddlemanEvent.java
+++ /dev/null
@@ -1,72 +0,0 @@
-// Copyright 2015 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.actions;
-
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
-
-/**
- * This event is fired during the build, when a middleman action is executed. Middleman actions
- * don't usually do any computation but we need them in the critical path because they depend on
- * other actions.
- */
-public final class ActionMiddlemanEvent implements Postable {
-
- private final Action action;
- private final InputMetadataProvider inputMetadataProvider;
- private final long nanoTimeStart;
- private final long nanoTimeFinish;
-
- /**
- * Create an event for action that has been started.
- *
- * @param action the middleman action.
- * @param inputMetadataProvider the metadata about the inputs of the action
- * @param nanoTimeStart the time when the action was started. This allow us to record more
- * accurately the time spent by the middleman action, since even for middleman actions we
- * execute some.
- */
- public ActionMiddlemanEvent(
- Action action,
- InputMetadataProvider inputMetadataProvider,
- long nanoTimeStart,
- long nanoTimeFinish) {
- Preconditions.checkArgument(action.getActionType().isMiddleman(),
- "Only middleman actions should be passed: %s", action);
- this.action = action;
- this.inputMetadataProvider = inputMetadataProvider;
- this.nanoTimeStart = nanoTimeStart;
- this.nanoTimeFinish = nanoTimeFinish;
- }
-
- /**
- * Returns the associated action.
- */
- public Action getAction() {
- return action;
- }
-
- /** Returns the metadata of the inputs of the action. */
- public InputMetadataProvider getInputMetadataProvider() {
- return inputMetadataProvider;
- }
-
- public long getNanoTimeStart() {
- return nanoTimeStart;
- }
-
- public long getNanoTimeFinish() {
- return nanoTimeFinish;
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD
index a0cb04a..050b6c4 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD
@@ -58,7 +58,6 @@
"ActionKeyContext.java",
"ActionLogBufferPathGenerator.java",
"ActionLookupValue.java",
- "ActionMiddlemanEvent.java",
"ActionOwner.java",
"ActionProgressEvent.java",
"ActionRegistry.java",
@@ -436,6 +435,7 @@
":file_metadata",
":runfiles_tree",
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
+ "//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:hash_codes",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
index 774175e..704972c 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
@@ -972,4 +972,46 @@
return "singleton marker artifact value (" + hashCode() + ")";
}
}
+
+ /** {@link FileArtifactValue} subclass for artifacts with constant metadata. A singleton. */
+ public static final class ConstantMetadataValue extends FileArtifactValue
+ implements FileArtifactValue.Singleton {
+ static final ConstantMetadataValue INSTANCE = new ConstantMetadataValue();
+ // This needs to not be of length 0, so it is distinguishable from a missing digest when written
+ // into a Fingerprint.
+ private static final byte[] DIGEST = new byte[1];
+
+ private ConstantMetadataValue() {}
+
+ @Override
+ public FileStateType getType() {
+ return FileStateType.REGULAR_FILE;
+ }
+
+ @Override
+ public byte[] getDigest() {
+ return DIGEST;
+ }
+
+ @Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public long getSize() {
+ return 0;
+ }
+
+ @Override
+ public long getModifiedTime() {
+ return -1;
+ }
+
+ @Override
+ public boolean wasModifiedSinceDigest(Path path) {
+ throw new UnsupportedOperationException(
+ "ConstantMetadataValue doesn't support wasModifiedSinceDigest " + path);
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java
index 6ca6e8d..94b3fd8 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanAction.java
@@ -22,6 +22,8 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.vfs.BulkDeleter;
+import com.google.devtools.build.lib.vfs.Path;
import javax.annotation.Nullable;
/**
@@ -53,7 +55,18 @@
@Override
public ActionResult execute(ActionExecutionContext actionExecutionContext) {
- throw new IllegalStateException("MiddlemanAction should never be executed");
+ return ActionResult.EMPTY;
+ }
+
+ @Override
+ public void prepare(
+ Path execRoot,
+ ArtifactPathResolver pathResolver,
+ @Nullable BulkDeleter bulkDeleter,
+ boolean cleanupArchivedArtifacts) {
+ // Runfiles trees are created as a side effect of building the output manifest, not the runfiles
+ // tree artifact. This method is overridden so that depending on the runfiles middleman does not
+ // delete the runfiles tree that's on the file system that someone decided it must be there.
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java
index c10146a..dca392b 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java
@@ -13,7 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.actions;
-/** The action type. */
+/** Deprecated, slated to be removed. */
public enum MiddlemanType {
/** A normal action. */
@@ -27,6 +27,9 @@
RUNFILES_MIDDLEMAN;
public boolean isMiddleman() {
- return this != NORMAL;
+ // This value is always false, which means that in theory, the MiddlemanType enum is not useful
+ // anymore. It's kept here to facilitate an easy rollback for the change that made the enum
+ // unnecessary should trouble arise.
+ return false;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java
index a3e933d..30ae791 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java
@@ -18,7 +18,9 @@
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.FileArtifactValue.ConstantMetadataValue;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
+import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.HashCodes;
import com.google.devtools.build.skyframe.SkyValue;
@@ -43,13 +45,11 @@
private final ImmutableList<TreeArtifactValue> treeValues;
public RunfilesArtifactValue(
- FileArtifactValue metadata,
RunfilesTree runfilesTree,
ImmutableList<Artifact> files,
ImmutableList<FileArtifactValue> fileValues,
ImmutableList<Artifact> trees,
ImmutableList<TreeArtifactValue> treeValues) {
- this.metadata = checkNotNull(metadata);
this.runfilesTree = checkNotNull(runfilesTree);
this.files = checkNotNull(files);
this.fileValues = checkNotNull(fileValues);
@@ -59,10 +59,41 @@
files.size() == fileValues.size() && trees.size() == treeValues.size(),
"Size mismatch: %s",
this);
+
+ // Compute the digest of this runfiles tree by combining its layout and the digests of every
+ // artifact it references.
+ this.metadata = FileArtifactValue.createProxy(computeDigest());
+ }
+
+ private byte[] computeDigest() {
+ Fingerprint result = new Fingerprint();
+
+ result.addInt(runfilesTree.getMapping().size());
+ for (var entry : runfilesTree.getMapping().entrySet()) {
+ result.addPath(entry.getKey());
+ result.addBoolean(entry.getValue() != null);
+ if (entry.getValue() != null) {
+ result.addPath(entry.getValue().getExecPath());
+ }
+ }
+
+ result.addInt(files.size());
+ for (int i = 0; i < files.size(); i++) {
+ FileArtifactValue value =
+ files.get(i).isConstantMetadata() ? ConstantMetadataValue.INSTANCE : fileValues.get(i);
+ value.addTo(result);
+ }
+
+ result.addInt(trees.size());
+ for (int i = 0; i < trees.size(); i++) {
+ result.addBytes(treeValues.get(i).getDigest());
+ }
+
+ return result.digestAndReset();
}
public RunfilesArtifactValue withOverriddenRunfilesTree(RunfilesTree overrideTree) {
- return new RunfilesArtifactValue(metadata, overrideTree, files, fileValues, trees, treeValues);
+ return new RunfilesArtifactValue(overrideTree, files, fileValues, trees, treeValues);
}
/** Returns the data of the artifact for this value, as computed by the action cache checker. */
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
index a7684e7..cb6cc8c 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java
@@ -19,7 +19,6 @@
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter;
import com.google.devtools.build.lib.actions.ActionLookupData;
-import com.google.devtools.build.lib.actions.MiddlemanType;
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.skyframe.ActionExecutionInactivityWatchdog;
import com.google.devtools.build.lib.skyframe.AspectCompletionValue;
@@ -184,7 +183,7 @@
}
private static boolean isActionReportWorthy(Action action) {
- return action.getActionType() == MiddlemanType.NORMAL;
+ return !action.getActionType().isMiddleman();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/metrics/criticalpath/CriticalPathComputer.java b/src/main/java/com/google/devtools/build/lib/metrics/criticalpath/CriticalPathComputer.java
index 6e5ad22..a6e6dff 100644
--- a/src/main/java/com/google/devtools/build/lib/metrics/criticalpath/CriticalPathComputer.java
+++ b/src/main/java/com/google/devtools/build/lib/metrics/criticalpath/CriticalPathComputer.java
@@ -26,7 +26,6 @@
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionCompletionEvent;
import com.google.devtools.build.lib.actions.ActionKeyContext;
-import com.google.devtools.build.lib.actions.ActionMiddlemanEvent;
import com.google.devtools.build.lib.actions.ActionStartedEvent;
import com.google.devtools.build.lib.actions.Actions;
import com.google.devtools.build.lib.actions.AggregatedSpawnMetrics;
@@ -235,23 +234,6 @@
}
/**
- * Record a middleman action execution. Even if middleman are almost instant, we record them
- * because they depend on other actions and we need them for constructing the critical path.
- *
- * <p>For some rules with incorrect configuration transitions we might get notified several times
- * for the same middleman. This should only happen if the actions are shared.
- */
- @Subscribe
- @AllowConcurrentEvents
- public void middlemanAction(ActionMiddlemanEvent event) throws InterruptedException {
- Action action = event.getAction();
- CriticalPathComponent component =
- tryAddComponent(createComponent(action, event.getNanoTimeStart()));
- finalizeActionStat(
- event.getNanoTimeStart(), event.getNanoTimeFinish(), action, component, "middleman action");
- }
-
- /**
* Try to add the component to the map of critical path components. If there is an existing
* component for its primary output it uses that to update the rest of the outputs.
*
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ExecutionGraphModule.java b/src/main/java/com/google/devtools/build/lib/runtime/ExecutionGraphModule.java
index f931f4b..ea5cfcf 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/ExecutionGraphModule.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/ExecutionGraphModule.java
@@ -29,7 +29,6 @@
import com.google.devtools.build.lib.actions.ActionCompletionEvent;
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.ActionMiddlemanEvent;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.CachedActionEvent;
@@ -181,7 +180,6 @@
private ActionDumpWriter writer;
private CommandEnvironment env;
- private ExecutionGraphOptions options;
private NanosToMillisSinceEpochConverter nanosToMillis =
BlazeClock.createNanosToMillisSinceEpochConverter();
// Only relevant for Skymeld: there may be multiple events and we only count the first one.
@@ -200,11 +198,6 @@
}
@VisibleForTesting
- void setOptions(ExecutionGraphOptions options) {
- this.options = options;
- }
-
- @VisibleForTesting
void setNanosToMillis(NanosToMillisSinceEpochConverter nanosToMillis) {
this.nanosToMillis = nanosToMillis;
}
@@ -234,7 +227,6 @@
BuildReport.newBuilder().setCode(Code.BUILD_REPORT_WRITE_FAILED))
.build())));
}
- this.options = options;
}
}
@@ -329,24 +321,6 @@
event.getNanoTimeFinish());
}
- /**
- * Record a middleman action execution. We may not needs this since we expand the runfiles
- * supplier inputs, but it's left here in case we need it.
- *
- * <p>TODO(vanja) remove this if it's not necessary.
- */
- @Subscribe
- @AllowConcurrentEvents
- public void middlemanAction(ActionMiddlemanEvent event) {
- if (options.logMiddlemanActions) {
- actionEvent(
- event.getAction(),
- event.getInputMetadataProvider(),
- event.getNanoTimeStart(),
- event.getNanoTimeFinish());
- }
- }
-
private void actionEvent(
Action action,
InputMetadataProvider inputMetadataProvider,
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 9b12ffa..167023d 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
@@ -191,8 +191,7 @@
"Null middleman action? %s",
artifactDependencies);
- FileArtifactValue individualMetadata = actionValue.getExistingFileArtifactValue(artifact);
- return createRunfilesArtifactValue(artifact, (MiddlemanAction) action, individualMetadata, env);
+ return createRunfilesArtifactValue(artifact, (MiddlemanAction) action, env);
}
private static void mkdirForTreeArtifact(
@@ -382,7 +381,6 @@
private static RunfilesArtifactValue createRunfilesArtifactValue(
Artifact artifact,
MiddlemanAction action,
- FileArtifactValue value,
SkyFunction.Environment env)
throws InterruptedException {
ImmutableList<Artifact> inputs = action.getInputs().toList();
@@ -425,7 +423,6 @@
}
return new RunfilesArtifactValue(
- value,
action.getRunfilesTree(),
files.build(),
fileValues.build(),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 6429b00..2ae3f90 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -43,7 +43,6 @@
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
import com.google.devtools.build.lib.actions.ActionLookupData;
-import com.google.devtools.build.lib.actions.ActionMiddlemanEvent;
import com.google.devtools.build.lib.actions.ActionOutputDirectoryHelper;
import com.google.devtools.build.lib.actions.ActionOutputDirectoryHelper.CreateOutputDirectoryException;
import com.google.devtools.build.lib.actions.ActionOwner;
@@ -688,13 +687,6 @@
if (token == null) {
boolean eventPosted = false;
- // Notify BlazeRuntimeStatistics about the action middleman 'execution'.
- if (action.getActionType().isMiddleman()) {
- eventHandler.post(
- new ActionMiddlemanEvent(
- action, inputMetadataProvider, actionStartTime, BlazeClock.nanoTime()));
- eventPosted = true;
- }
if (action instanceof NotifyOnActionCacheHit notify) {
ExtendedEventHandler contextEventHandler = selectEventHandler(action);
