Clean up artifact retrieval methods in ActionExecutionValue.
All callers of getArtifactValue were doing a checkNotNull on the return value, which is exactly what getExistingFileArtifactValue does. Migrate them so that we only need one retrieval method. Loosen the parameter to Artifact and do the instanceof check in one place, so any bugs get an appropriate error message.
Additionally, use the recently added isChildOfDeclaredDirectory method to determine which map to search for the artifact, instead of unnecessarily checking both.
RELNOTES: None.
PiperOrigin-RevId: 314380367
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 3ea28f6..50f1c35 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
@@ -41,7 +41,6 @@
import com.google.devtools.build.lib.actions.ActionRewoundEvent;
import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.DiscoveredInputsEvent;
import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -1076,9 +1075,7 @@
inputData.putWithNoDepOwner(input, treeValue.getSelfData());
} else if (retrievedMetadata instanceof ActionExecutionValue) {
inputData.putWithNoDepOwner(
- input,
- ((ActionExecutionValue) retrievedMetadata)
- .getExistingFileArtifactValue((DerivedArtifact) input));
+ input, ((ActionExecutionValue) retrievedMetadata).getExistingFileArtifactValue(input));
} else if (retrievedMetadata instanceof MissingFileArtifactValue) {
inputData.putWithNoDepOwner(input, FileArtifactValue.MISSING_FILE_MARKER);
} else if (retrievedMetadata instanceof FileArtifactValue) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index 1c63e3e..b097fe1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -125,43 +125,34 @@
}
/**
- * Retrieves a {@link FileArtifactValue} for a regular (non-middleman, non-tree) derived artifact.
+ * Retrieves a {@link FileArtifactValue} for a regular (non-tree) derived artifact.
*
- * <p>The value for the given artifact must be present.
+ * <p>The value for the given artifact must be present, or else {@link NullPointerException} will
+ * be thrown.
*/
- FileArtifactValue getExistingFileArtifactValue(DerivedArtifact artifact) {
- Preconditions.checkState(
- !artifact.isMiddlemanArtifact() && !artifact.isTreeArtifact(),
+ public FileArtifactValue getExistingFileArtifactValue(Artifact artifact) {
+ Preconditions.checkArgument(
+ artifact instanceof DerivedArtifact && !artifact.isTreeArtifact(),
"Cannot request %s from %s",
artifact,
this);
+
+ FileArtifactValue result;
+ if (artifact.isChildOfDeclaredDirectory()) {
+ TreeArtifactValue tree = treeArtifactData.get(artifact.getParent());
+ result = tree == null ? null : tree.getChildValues().get(artifact);
+ } else {
+ result = artifactData.get(artifact);
+ }
+
return Preconditions.checkNotNull(
- getArtifactValue(artifact),
+ result,
"Missing artifact %s (generating action key %s) in %s",
artifact,
- artifact.getGeneratingActionKey(),
+ ((DerivedArtifact) artifact).getGeneratingActionKey(),
this);
}
- /**
- * @return The data for each non-middleman output of this action, in the form of the {@link
- * com.google.devtools.build.lib.actions.FileValue} that would be created for the file if it
- * were to be read from disk.
- */
- @Nullable
- public FileArtifactValue getArtifactValue(Artifact artifact) {
- FileArtifactValue result = artifactData.get(artifact);
- if (result != null || !artifact.hasParent()) {
- return result;
- }
- // In some cases, TreeFileArtifact metadata may not have been injected directly, and is only
- // available via the parent. However, if this ActionExecutionValue corresponds to a templated
- // action, as opposed to an action that created a tree artifact itself, the TreeFileArtifact
- // metadata will be in artifactData, since this value will have no treeArtifactData.
- TreeArtifactValue treeArtifactValue = treeArtifactData.get(artifact.getParent());
- return treeArtifactValue == null ? null : treeArtifactValue.getChildValues().get(artifact);
- }
-
TreeArtifactValue getTreeArtifactValue(Artifact artifact) {
Preconditions.checkArgument(artifact.isTreeArtifact());
return treeArtifactData.get(artifact);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
index 03fed1a..5f1d89e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
@@ -33,10 +33,15 @@
import java.util.Collection;
import java.util.Map;
-class ActionInputMapHelper {
+/** Static utilities for working with action inputs. */
+final class ActionInputMapHelper {
- // Adds a value obtained by an Artifact skyvalue lookup to the action input map. May do Skyframe
- // lookups.
+ private ActionInputMapHelper() {}
+
+ /**
+ * Adds a value obtained by an Artifact skyvalue lookup to the action input map. May do Skyframe
+ * lookups.
+ */
static void addToMap(
ActionInputMapSink inputMap,
Map<Artifact, Collection<Artifact>> expandedArtifacts,
@@ -87,10 +92,7 @@
expandTreeArtifactAndPopulateArtifactData(
key, (TreeArtifactValue) value, expandedArtifacts, inputMap, /*depOwner=*/ key);
} else if (value instanceof ActionExecutionValue) {
- inputMap.put(
- key,
- ((ActionExecutionValue) value).getExistingFileArtifactValue((DerivedArtifact) key),
- key);
+ inputMap.put(key, ((ActionExecutionValue) value).getExistingFileArtifactValue(key), key);
if (key.isFileset()) {
topLevelFilesets.put(key, getFilesets(env, (SpecialArtifact) key));
}
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 d9f50b4..9f83aa2 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
@@ -133,9 +133,7 @@
artifactDependencies.actionLookupValue.getAction(generatingActionKey.getActionIndex()),
"Null middleman action? %s",
artifactDependencies);
- FileArtifactValue individualMetadata =
- Preconditions.checkNotNull(
- actionValue.getArtifactValue(artifact), "%s %s", artifact, actionValue);
+ FileArtifactValue individualMetadata = actionValue.getExistingFileArtifactValue(artifact);
if (isAggregatingValue(action)) {
return createAggregatingValue(artifact, action, individualMetadata, env);
}
@@ -331,9 +329,7 @@
} else if (inputValue instanceof ActionExecutionValue) {
fileInputsBuilder.add(
Pair.of(
- input,
- ((ActionExecutionValue) inputValue)
- .getExistingFileArtifactValue((DerivedArtifact) input)));
+ input, ((ActionExecutionValue) inputValue).getExistingFileArtifactValue(input)));
} else if (inputValue instanceof TreeArtifactValue) {
directoryInputsBuilder.add(Pair.of(input, (TreeArtifactValue) inputValue));
} else {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
index 76914f4..9e3932e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -26,7 +26,6 @@
import com.google.common.flogger.GoogleLogger;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileStateType;
@@ -337,8 +336,7 @@
Pair<SkyKey, ActionExecutionValue> keyAndValue = fileToKeyAndValue.get(artifact);
ActionExecutionValue actionValue = keyAndValue.getSecond();
SkyKey key = keyAndValue.getFirst();
- FileArtifactValue lastKnownData =
- actionValue.getExistingFileArtifactValue((DerivedArtifact) artifact);
+ FileArtifactValue lastKnownData = actionValue.getExistingFileArtifactValue(artifact);
try {
FileArtifactValue newData =
ActionMetadataHandler.fileArtifactValueFromArtifact(artifact, stat, tsgm);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
index f260515..c611700 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
@@ -22,7 +22,6 @@
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileStateType;
@@ -278,9 +277,7 @@
if (value instanceof FileArtifactValue || value instanceof TreeArtifactValue) {
fsVal = (HasDigest) value;
} else if (value instanceof ActionExecutionValue) {
- fsVal =
- ((ActionExecutionValue) value)
- .getExistingFileArtifactValue((DerivedArtifact) artifact);
+ fsVal = ((ActionExecutionValue) value).getExistingFileArtifactValue(artifact);
} else {
return NON_EXISTENT_FILE_INFO;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
index 70e9eba..e77a62c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
@@ -480,11 +480,12 @@
assertThat(treeValue.getChildPaths())
.containsExactly(PathFragment.create("foo"), PathFragment.create("bar"));
assertThat(treeValue.getChildValues().values()).containsExactly(fooValue, barValue);
+
+ // Make sure that all children are transferred properly into the ActionExecutionValue. If any
+ // child is missing, getExistingFileArtifactValue will throw.
ActionExecutionValue actionExecutionValue =
ActionExecutionValue.createFromOutputStore(handler.getOutputStore(), null, null, false);
- treeValue
- .getChildren()
- .forEach(t -> assertThat(actionExecutionValue.getArtifactValue(t)).isNotNull());
+ treeValue.getChildren().forEach(actionExecutionValue::getExistingFileArtifactValue);
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 5607782..67924f1 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -434,8 +434,7 @@
}
SkyValue value = result.get(key);
if (value instanceof ActionExecutionValue) {
- return ((ActionExecutionValue) value)
- .getExistingFileArtifactValue((DerivedArtifact) artifact);
+ return ((ActionExecutionValue) value).getExistingFileArtifactValue(artifact);
}
return value;
}
