Remove {OutputStore,ActionExecutionValue}.additionalOutputData.
Instead of having another map, when additional data is needed, replace artifactData with it.
There was one place where we exploited the difference between the two: for cases where we get a digest from remote execution but one is not available for the file system, FilesystemValueChecker used only one map.
I fixed this by adding a method to FileArtifactValue that tells whether given two instances, Blaze should assume that there was no change and replicated the existing behavior as faithfully as possible.
RELNOTES: None.
PiperOrigin-RevId: 261086332
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 d7cbc28..5ab0b65 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
@@ -56,13 +56,6 @@
@Immutable
@ThreadSafe
public abstract class FileArtifactValue implements SkyValue, HasDigest {
-
- /**
- * Used as as placeholder in {@code OutputStore.artifactData} for artifacts that have entries in
- * {@code OutputStore.additionalOutputData}.
- */
- @AutoCodec public static final FileArtifactValue PLACEHOLDER = new PlaceholderFileValue();
-
/**
* The type of the underlying file system object. If it is a regular file, then it is guaranteed
* to have a digest. Otherwise it does not have a digest.
@@ -71,8 +64,8 @@
/**
* Returns a digest of the content of the underlying file system object; must always return a
- * non-null value for instances of type {@link FileStateType#REGULAR_FILE}. Otherwise may return
- * null.
+ * non-null value for instances of type {@link FileStateType#REGULAR_FILE} that are owned by an
+ * {@code ActionExecutionValue}.
*
* <p>All instances of this interface must either have a digest or return a last-modified time.
* Clients should prefer using the digest for content identification (e.g., for caching), and only
@@ -93,6 +86,9 @@
*/
public abstract long getModifiedTime();
+ // TODO(lberki): This is only used by FileArtifactValue itself. It seems possible to remove this.
+ public abstract FileContentsProxy getContentsProxy();
+
@Nullable
@Override
public BigInteger getValueFingerprint() {
@@ -120,6 +116,11 @@
return this instanceof Singleton;
}
+ /** Returns {@code true} if the file only exists remotely. */
+ public boolean isRemote() {
+ return false;
+ }
+
/**
* Provides a best-effort determination whether the file was changed since the digest was
* computed. This method performs file system I/O, so may be expensive. It's primarily intended to
@@ -127,11 +128,28 @@
* that the file was modified since the digest was computed. Better not upload if we are not sure
* that the cache entry is reliable.
*/
+ // TODO(lberki): This is very similar to couldBeModifiedSince(). Check if we can unify these.
public abstract boolean wasModifiedSinceDigest(Path path) throws IOException;
- /** Returns {@code true} if the file only exists remotely. */
- public boolean isRemote() {
- return false;
+ /**
+ * Returns whether the two {@link FileArtifactValue} instances could be considered the same for
+ * purposes of action invalidation.
+ */
+ // TODO(lberki): This is very similar to wasModifiedSinceDigest(). Check if we can unify these.
+ public boolean couldBeModifiedSince(FileArtifactValue lastKnown) {
+ if (this instanceof Singleton || lastKnown instanceof Singleton) {
+ return true;
+ }
+
+ if (getType() != lastKnown.getType()) {
+ return true;
+ }
+
+ if (getDigest() != null && lastKnown.getDigest() != null) {
+ return !Arrays.equals(getDigest(), lastKnown.getDigest()) || getSize() != lastKnown.getSize();
+ } else {
+ return getModifiedTime() != lastKnown.getModifiedTime();
+ }
}
@Override
@@ -169,8 +187,6 @@
}
}
- public abstract FileContentsProxy getContentsProxy();
-
/**
* Marker interface for singleton implementations of this class.
*
@@ -384,7 +400,7 @@
}
@Override
- public boolean wasModifiedSinceDigest(Path path) throws IOException {
+ public boolean wasModifiedSinceDigest(Path path) {
// TODO(ulfjack): Ideally, we'd attempt to detect intra-build modifications here. I'm
// consciously deferring work here as this code will most likely change again, and we're
// already doing better than before by detecting inter-build modifications.
@@ -397,6 +413,16 @@
}
@Override
+ public boolean couldBeModifiedSince(FileArtifactValue o) {
+ if (!(o instanceof HashedDirectoryArtifactValue)) {
+ return true;
+ }
+
+ HashedDirectoryArtifactValue lastKnown = (HashedDirectoryArtifactValue) o;
+ return !Arrays.equals(digest, lastKnown.digest);
+ }
+
+ @Override
public boolean equals(Object o) {
if (this == o) {
return true;
@@ -471,6 +497,24 @@
}
@Override
+ public boolean couldBeModifiedSince(FileArtifactValue o) {
+ if (!(o instanceof RegularFileArtifactValue)) {
+ return true;
+ }
+
+ RegularFileArtifactValue lastKnown = (RegularFileArtifactValue) o;
+ if (size != lastKnown.size || dataIsShareable() != lastKnown.dataIsShareable()) {
+ return true;
+ }
+
+ if (digest != null && lastKnown.digest != null) {
+ return !Arrays.equals(digest, lastKnown.digest);
+ } else {
+ return !Objects.equals(proxy, lastKnown.proxy);
+ }
+ }
+
+ @Override
public boolean equals(Object o) {
if (this == o) {
return true;
@@ -718,7 +762,7 @@
}
@Override
- public boolean wasModifiedSinceDigest(Path path) throws IOException {
+ public boolean wasModifiedSinceDigest(Path path) {
return false;
}
@@ -770,61 +814,4 @@
return "OMITTED_FILE_MARKER";
}
}
-
- private static final class PlaceholderFileValue extends FileArtifactValue implements Singleton {
- private static final BigInteger FINGERPRINT =
- new BigIntegerFingerprint().addString("PlaceholderFileValue").getFingerprint();
-
- private PlaceholderFileValue() {}
-
- @Override
- public long getSize() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public byte[] getDigest() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public FileContentsProxy getContentsProxy() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public BigInteger getValueFingerprint() {
- return FINGERPRINT;
- }
-
- @Override
- public String toString() {
- return "PlaceholderFileValue:Singleton";
- }
-
- @Override
- public FileStateType getType() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public long getModifiedTime() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public boolean wasModifiedSinceDigest(Path path) {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public boolean isRemote() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public boolean isMarkerValue() {
- throw new UnsupportedOperationException();
- }
- }
}
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 ffc04336..4de2fbb 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
@@ -19,12 +19,13 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Maps;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileStateType;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -43,37 +44,17 @@
/**
* A value representing an executed action.
- *
- * <p>Concerning the data in this class:
- *
- * <p>We want to track all output data from an ActionExecutionValue. See {@link OutputStore} for a
- * discussion of how its fields {@link OutputStore#artifactData} and {@link
- * OutputStore#additionalOutputData} relate. The relationship is the same between {@link
- * #artifactData} and {@link #additionalOutputData} here.
*/
@Immutable
@ThreadSafe
public class ActionExecutionValue implements SkyValue {
- /**
- * The metadata of all files for this ActionExecutionValue whose filesystem metadata differs from
- * the metadata in {@link #additionalOutputData} or is not present there. This metadata can be
- * checked quickly against the actual filesystem on incremental builds.
- *
- * <p>If additional metadata is not needed here over what is in {@link #additionalOutputData}, the
- * value will be {@link FileArtifactValue#PLACEHOLDER}.
- */
+ /** A map from each output artifact of this action to their {@link FileArtifactValue}s. */
private final ImmutableMap<Artifact, FileArtifactValue> artifactData;
/** The TreeArtifactValue of all TreeArtifacts output by this Action. */
private final ImmutableMap<Artifact, TreeArtifactValue> treeArtifactData;
- /**
- * Contains all remaining data that weren't in the above maps. See {@link
- * OutputStore#getAllAdditionalOutputData}.
- */
- private final ImmutableMap<Artifact, FileArtifactValue> additionalOutputData;
-
@Nullable private final ImmutableList<FilesetOutputSymlink> outputSymlinks;
@Nullable private final NestedSet<Artifact> discoveredModules;
@@ -86,21 +67,38 @@
/**
* @param artifactData Map from Artifacts to corresponding {@link FileArtifactValue}.
* @param treeArtifactData All tree artifact data.
- * @param additionalOutputData Map from Artifacts to values if the FileArtifactValue for this
- * artifact cannot be derived from the corresponding FileValue (see {@link
- * OutputStore#getAllAdditionalOutputData} for when this is necessary). These output data are
- * not used by the {@link FilesystemValueChecker} to invalidate ActionExecutionValues.
* @param outputSymlinks This represents the SymlinkTree which is the output of a fileset action.
* @param discoveredModules cpp modules discovered
*/
private ActionExecutionValue(
Map<Artifact, FileArtifactValue> artifactData,
Map<Artifact, TreeArtifactValue> treeArtifactData,
- Map<Artifact, FileArtifactValue> additionalOutputData,
@Nullable ImmutableList<FilesetOutputSymlink> outputSymlinks,
@Nullable NestedSet<Artifact> discoveredModules) {
+ for (Map.Entry<Artifact, FileArtifactValue> entry : artifactData.entrySet()) {
+ if (entry.getValue().getType() == FileStateType.REGULAR_FILE) {
+ Preconditions.checkArgument(
+ entry.getValue().getDigest() != null, "missing digest for %s", entry.getKey());
+ }
+ }
+
+ for (Map.Entry<Artifact, TreeArtifactValue> tree : treeArtifactData.entrySet()) {
+ for (Map.Entry<TreeFileArtifact, FileArtifactValue> file :
+ tree.getValue().getChildValues().entrySet()) {
+ Preconditions.checkArgument(
+ file.getValue().getType() == FileStateType.REGULAR_FILE,
+ "file %s in tree artifact %s is not a regular file",
+ file.getKey(),
+ tree.getKey());
+ Preconditions.checkArgument(
+ file.getValue().getDigest() != null,
+ "missing digest for file %s in tree artifact %s",
+ file.getKey(),
+ tree.getKey());
+ }
+ }
+
this.artifactData = ImmutableMap.copyOf(artifactData);
- this.additionalOutputData = ImmutableMap.copyOf(additionalOutputData);
this.treeArtifactData = ImmutableMap.copyOf(treeArtifactData);
this.outputSymlinks = outputSymlinks;
this.discoveredModules = discoveredModules;
@@ -114,7 +112,6 @@
return create(
outputStore.getAllArtifactData(),
outputStore.getAllTreeArtifactData(),
- outputStore.getAllAdditionalOutputData(),
outputSymlinks,
discoveredModules,
actionDependsOnBuildId);
@@ -123,38 +120,22 @@
static ActionExecutionValue create(
Map<Artifact, FileArtifactValue> artifactData,
Map<Artifact, TreeArtifactValue> treeArtifactData,
- Map<Artifact, FileArtifactValue> additionalOutputData,
@Nullable ImmutableList<FilesetOutputSymlink> outputSymlinks,
@Nullable NestedSet<Artifact> discoveredModules,
boolean actionDependsOnBuildId) {
return actionDependsOnBuildId
? new CrossServerUnshareableActionExecutionValue(
- artifactData, treeArtifactData, additionalOutputData, outputSymlinks, discoveredModules)
+ artifactData, treeArtifactData, outputSymlinks, discoveredModules)
: new ActionExecutionValue(
- artifactData,
- treeArtifactData,
- additionalOutputData,
- outputSymlinks,
- discoveredModules);
- }
-
- /**
- * Returns metadata for a given artifact, if that metadata cannot be inferred from the
- * corresponding {@link #getData} call for that Artifact. See {@link
- * OutputStore#getAllAdditionalOutputData} for when that can happen.
- */
- @Nullable
- public FileArtifactValue getArtifactValue(Artifact artifact) {
- return additionalOutputData.get(artifact);
+ artifactData, treeArtifactData, outputSymlinks, discoveredModules);
}
/**
* @return The data for each non-middleman output of this action, in the form of the {@link
* FileValue} that would be created for the file if it were to be read from disk.
*/
- FileArtifactValue getData(Artifact artifact) {
- Preconditions.checkState(!additionalOutputData.containsKey(artifact),
- "Should not be requesting data for already-constructed FileArtifactValue: %s", artifact);
+ @Nullable
+ public FileArtifactValue getArtifactValue(Artifact artifact) {
return artifactData.get(artifact);
}
@@ -165,11 +146,11 @@
/**
* @return The map from {@link Artifact}s to the corresponding {@link FileValue}s that would be
- * returned by {@link #getData}. Primarily needed by {@link FilesystemValueChecker}, also
- * called by {@link ArtifactFunction} when aggregating a {@link TreeArtifactValue}.
+ * returned by {@link #getArtifactValue}. Primarily needed by {@link FilesystemValueChecker},
+ * also called by {@link ArtifactFunction} when aggregating a {@link TreeArtifactValue}.
*/
Map<Artifact, FileArtifactValue> getAllFileValues() {
- return Maps.transformEntries(artifactData, this::transformIfPlaceholder);
+ return artifactData;
}
/**
@@ -207,12 +188,6 @@
fp.addPath(entry.getKey().getExecPath());
fp.addBigIntegerOrdered(entry.getValue().getValueFingerprint());
});
- sortMapByArtifactExecPathAndStream(additionalOutputData)
- .forEach(
- (entry) -> {
- fp.addPath(entry.getKey().getExecPath());
- fp.addBigIntegerOrdered(entry.getValue().getValueFingerprint());
- });
if (outputSymlinks != null) {
for (FilesetOutputSymlink symlink : outputSymlinks) {
fp.addBigIntegerOrdered(symlink.getFingerprint());
@@ -234,7 +209,6 @@
return MoreObjects.toStringHelper(this)
.add("artifactData", artifactData)
.add("treeArtifactData", treeArtifactData)
- .add("additionalOutputData", additionalOutputData)
.toString();
}
@@ -252,31 +226,12 @@
ActionExecutionValue o = (ActionExecutionValue) obj;
return artifactData.equals(o.artifactData)
&& treeArtifactData.equals(o.treeArtifactData)
- && additionalOutputData.equals(o.additionalOutputData)
&& (outputSymlinks == null || outputSymlinks.equals(o.outputSymlinks));
}
@Override
public int hashCode() {
- return Objects.hashCode(artifactData, treeArtifactData, additionalOutputData);
- }
-
- /** Transforms PLACEHOLDER values into normal instances. */
- private FileArtifactValue transformIfPlaceholder(Artifact artifact, FileArtifactValue value) {
- Preconditions.checkState(!artifact.isTreeArtifact());
- Preconditions.checkState(!artifact.isMiddlemanArtifact());
-
- if (value != FileArtifactValue.PLACEHOLDER) {
- return value;
- }
-
- FileArtifactValue metadata =
- Preconditions.checkNotNull(
- additionalOutputData.get(artifact),
- "Placeholder without corresponding FileArtifactValue for: %s",
- artifact);
-
- return metadata;
+ return Objects.hashCode(artifactData, treeArtifactData);
}
/**
@@ -288,11 +243,9 @@
CrossServerUnshareableActionExecutionValue(
Map<Artifact, FileArtifactValue> artifactData,
Map<Artifact, TreeArtifactValue> treeArtifactData,
- Map<Artifact, FileArtifactValue> additionalOutputData,
@Nullable ImmutableList<FilesetOutputSymlink> outputSymlinks,
@Nullable NestedSet<Artifact> discoveredModules) {
- super(
- artifactData, treeArtifactData, additionalOutputData, outputSymlinks, discoveredModules);
+ super(artifactData, treeArtifactData, outputSymlinks, discoveredModules);
}
@Override
@@ -366,7 +319,6 @@
return create(
transformKeys(artifactData, newArtifactMap),
transformKeys(treeArtifactData, newArtifactMap),
- transformKeys(additionalOutputData, newArtifactMap),
outputSymlinks,
discoveredModules,
this instanceof CrossServerUnshareableActionExecutionValue);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index 97ce7c6..f04d2e7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -71,10 +71,7 @@
* run, to gather information about the outputs. Second, it is accessed by {@link ArtifactFunction}s
* in order to construct {@link FileArtifactValue}s, and by this class itself to generate {@link
* TreeArtifactValue}s. Third, the {@link FilesystemValueChecker} uses it to determine the set of
- * output files to check for inter-build modifications. Because all these use cases are slightly
- * different, we must occasionally store two versions of the data for a value. See {@link
- * OutputStore#getAllAdditionalOutputData} for elaboration on the difference between these cases,
- * and see the javadoc for the various internal maps to see what is stored where.
+ * output files to check for inter-build modifications.
*/
@VisibleForTesting
public final class ActionMetadataHandler implements MetadataHandler {
@@ -188,12 +185,12 @@
} else if (artifact.isMiddlemanArtifact()) {
// A middleman artifact's data was either already injected from the action cache checker using
// #setDigestForVirtualArtifact, or it has the default middleman value.
- value = store.getAdditionalOutputData(artifact);
+ value = store.getArtifactData(artifact);
if (value != null) {
return metadataFromValue(value);
}
value = FileArtifactValue.DEFAULT_MIDDLEMAN;
- store.putAdditionalOutputData(artifact, value);
+ store.putArtifactData(artifact, value);
return metadataFromValue(value);
} else if (artifact.isTreeArtifact()) {
TreeArtifactValue setValue = getTreeArtifactValue((SpecialArtifact) artifact);
@@ -220,19 +217,7 @@
// results are then stored in Skyframe (and the action cache).
FileArtifactValue fileMetadata = store.getArtifactData(artifact);
if (fileMetadata != null) {
- // Non-middleman artifacts should only have additionalOutputData if they have
- // outputArtifactData. We don't assert this because of concurrency possibilities, but at least
- // we don't check additionalOutputData unless we expect that we might see the artifact there.
- value = store.getAdditionalOutputData(artifact);
- // If additional output data is present for this artifact, we use it in preference to the
- // usual calculation.
- if (value != null) {
- return metadataFromValue(value);
- }
- if (fileMetadata.getType() == FileStateType.NONEXISTENT) {
- throw new FileNotFoundException(artifact.prettyPrint() + " does not exist");
- }
- return fileMetadata;
+ return metadataFromValue(fileMetadata);
}
// No existing metadata; this can happen if the output metadata is not injected after a spawn
@@ -256,10 +241,6 @@
return inputArtifactData.getInput(execPath);
}
- /**
- * See {@link OutputStore#getAllAdditionalOutputData} for why we sometimes need to store
- * additional data, even for normal (non-middleman) artifacts.
- */
private FileArtifactValue maybeStoreAdditionalData(
Artifact artifact, FileArtifactValue data, @Nullable byte[] injectedDigest)
throws IOException {
@@ -303,7 +284,7 @@
data, injectedDigest, !artifact.isConstantMetadata());
}
- store.putAdditionalOutputData(artifact, value);
+ store.putArtifactData(artifact, value);
return metadataFromValue(value);
}
@@ -311,7 +292,7 @@
public void setDigestForVirtualArtifact(Artifact artifact, Md5Digest md5Digest) {
Preconditions.checkArgument(artifact.isMiddlemanArtifact(), artifact);
Preconditions.checkNotNull(md5Digest, artifact);
- store.putAdditionalOutputData(
+ store.putArtifactData(
artifact, FileArtifactValue.createProxy(md5Digest.getDigestBytesUnsafe()));
}
@@ -380,32 +361,30 @@
Maps.newHashMapWithExpectedSize(contents.size());
for (TreeFileArtifact treeFileArtifact : contents) {
- FileArtifactValue cachedValue = store.getAdditionalOutputData(treeFileArtifact);
- if (cachedValue == null) {
- FileArtifactValue fileMetadata = store.getArtifactData(treeFileArtifact);
- // This is similar to what's present in getRealMetadataForArtifact, except
- // we get back the ArtifactFileMetadata, not the metadata.
- // We do not cache exceptions besides nonexistence here, because it is unlikely that the
- // file will be requested from this cache too many times.
- if (fileMetadata == null) {
- try {
- fileMetadata = constructFileArtifactValue(treeFileArtifact, /*statNoFollow=*/ null);
- } catch (FileNotFoundException e) {
- String errorMessage = String.format(
- "Failed to resolve relative path %s inside TreeArtifact %s. "
- + "The associated file is either missing or is an invalid symlink.",
- treeFileArtifact.getParentRelativePath(),
- treeFileArtifact.getParent().getExecPathString());
- throw new IOException(errorMessage, e);
- }
+ FileArtifactValue fileMetadata = store.getArtifactData(treeFileArtifact);
+ // This is similar to what's present in getRealMetadataForArtifact, except
+ // we get back the ArtifactFileMetadata, not the metadata.
+ // We do not cache exceptions besides nonexistence here, because it is unlikely that the
+ // file will be requested from this cache too many times.
+ if (fileMetadata == null) {
+ try {
+ fileMetadata = constructFileArtifactValue(treeFileArtifact, /*statNoFollow=*/ null);
+ } catch (FileNotFoundException e) {
+ String errorMessage =
+ String.format(
+ "Failed to resolve relative path %s inside TreeArtifact %s. "
+ + "The associated file is either missing or is an invalid symlink.",
+ treeFileArtifact.getParentRelativePath(),
+ treeFileArtifact.getParent().getExecPathString());
+ throw new IOException(errorMessage, e);
}
// A minor hack: maybeStoreAdditionalData will force the data to be stored via
// store.putAdditionalOutputData, if the underlying OutputStore supports it.
- cachedValue = maybeStoreAdditionalData(treeFileArtifact, fileMetadata, null);
+ fileMetadata = maybeStoreAdditionalData(treeFileArtifact, fileMetadata, null);
}
- values.put(treeFileArtifact, cachedValue);
+ values.put(treeFileArtifact, fileMetadata);
}
return TreeArtifactValue.create(values);
@@ -537,7 +516,7 @@
if (output instanceof Artifact) {
Artifact artifact = (Artifact) output;
Preconditions.checkState(omittedOutputs.add(artifact), artifact);
- store.putAdditionalOutputData(artifact, FileArtifactValue.OMITTED_FILE_MARKER);
+ store.putArtifactData(artifact, FileArtifactValue.OMITTED_FILE_MARKER);
}
}
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 0e32d6f..b927f63 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
@@ -295,7 +295,8 @@
return value;
}
FileArtifactValue data =
- Preconditions.checkNotNull(actionValue.getData(artifact), "%s %s", artifact, actionValue);
+ Preconditions.checkNotNull(
+ actionValue.getArtifactValue(artifact), "%s %s", artifact, actionValue);
Preconditions.checkNotNull(
data.getDigest(), "Digest should already have been calculated for %s (%s)", artifact, data);
// Directories are special-cased because their mtimes are used, so should have been constructed
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 40bf9fd..e94cbd0 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
@@ -308,7 +308,7 @@
try {
FileArtifactValue newData =
ActionMetadataHandler.fileArtifactValueFromArtifact(artifact, stat, tsgm);
- if (!newData.equals(lastKnownData)) {
+ if (newData.couldBeModifiedSince(lastKnownData)) {
updateIntraBuildModifiedCounter(stat != null ? stat.getLastChangeTime() : -1);
modifiedOutputFilesCounter.getAndIncrement();
dirtyKeys.add(key);
@@ -406,7 +406,7 @@
for (Map.Entry<Artifact, FileArtifactValue> entry : actionValue.getAllFileValues().entrySet()) {
Artifact file = entry.getKey();
FileArtifactValue lastKnownData = entry.getValue();
- if (shouldCheckFile(knownModifiedOutputFiles, file)) {
+ if (!file.isMiddlemanArtifact() && shouldCheckFile(knownModifiedOutputFiles, file)) {
try {
FileArtifactValue fileMetadata =
ActionMetadataHandler.fileArtifactValueFromArtifact(file, null, tsgm);
@@ -414,7 +414,7 @@
boolean lastSeenRemotely = (fileValue != null) && fileValue.isRemote();
boolean trustRemoteValue =
fileMetadata.getType() == FileStateType.NONEXISTENT && lastSeenRemotely;
- if (!trustRemoteValue && !fileMetadata.equals(lastKnownData)) {
+ if (!trustRemoteValue && fileMetadata.couldBeModifiedSince(lastKnownData)) {
updateIntraBuildModifiedCounter(
fileMetadata.getType() != FileStateType.NONEXISTENT
? file.getPath().getLastModifiedTime(Symlinks.FOLLOW)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
index 0c9b9ca..d12823b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
@@ -25,17 +25,13 @@
* be worthwhile.
*/
final class MinimalOutputStore extends OutputStore {
-
@Override
- void putAdditionalOutputData(Artifact artifact, FileArtifactValue value) {
+ void putArtifactData(Artifact artifact, FileArtifactValue value) {
if (value.isMarkerValue()) {
- super.putAdditionalOutputData(artifact, value);
+ super.putArtifactData(artifact, value);
}
}
@Override
- void putArtifactData(Artifact artifact, FileArtifactValue value) {}
-
- @Override
void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) {}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
index ed034bd..bc75258 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
@@ -28,12 +28,12 @@
/**
* Storage layer for data associated with outputs of an action.
*
- * <p>Data is mainly stored in four maps, {@link #artifactData}, {@link #treeArtifactData}, {@link
- * #additionalOutputData}, and {@link #treeArtifactContents}, all of which are keyed on an {@link
- * Artifact}. For each of these maps, this class exposes standard methods such as {@code get},
- * {@code put}, {@code add}, and {@code getAll}.
+ * <p>Data is mainly stored in three maps, {@link #artifactData}, {@link #treeArtifactData} and
+ * {@link #treeArtifactContents}, all of which are keyed on an {@link Artifact}. For each of these
+ * maps, this class exposes standard methods such as {@code get}, {@code put}, {@code add}, and
+ * {@code getAll}.
*
- * <p>This implementation aggresively stores all data. Subclasses may override mutating methods to
+ * <p>This implementation aggressively stores all data. Subclasses may override mutating methods to
* avoid storing unnecessary data.
*/
@ThreadSafe
@@ -44,9 +44,6 @@
private final ConcurrentMap<Artifact, TreeArtifactValue> treeArtifactData =
new ConcurrentHashMap<>();
- private final ConcurrentMap<Artifact, FileArtifactValue> additionalOutputData =
- new ConcurrentHashMap<>();
-
private final ConcurrentMap<Artifact, Set<TreeFileArtifact>> treeArtifactContents =
new ConcurrentHashMap<>();
@@ -61,12 +58,6 @@
artifactData.put(artifact, value);
}
- /**
- * Returns data for output files that was computed during execution.
- *
- * <p>The value is {@link FileArtifactValue#PLACEHOLDER} if the artifact's metadata is not fully
- * captured in {@link #additionalOutputData}.
- */
final ImmutableMap<Artifact, FileArtifactValue> getAllArtifactData() {
return ImmutableMap.copyOf(artifactData);
}
@@ -88,51 +79,6 @@
return ImmutableMap.copyOf(treeArtifactData);
}
- @Nullable
- final FileArtifactValue getAdditionalOutputData(Artifact artifact) {
- return additionalOutputData.get(artifact);
- }
-
- void putAdditionalOutputData(Artifact artifact, FileArtifactValue value) {
- additionalOutputData.put(artifact, value);
- }
-
- /**
- * Returns data for any output files whose metadata was not computable from the corresponding
- * entry in {@link #artifactData}.
- *
- * <p>There are two bits to consider: the filesystem possessing fast digests and the execution
- * service injecting metadata via {@link ActionMetadataHandler#injectRemoteFile} or {@link
- * ActionMetadataHandler#injectDigest}.
- *
- * <ol>
- * <li>If the filesystem does not possess fast digests, then we will have additional output data
- * for practically every artifact, since we will need to store their digests.
- * <li>If we have a remote execution service injecting metadata, then we will just store that
- * metadata here, and put {@link FileArtifactValue#PLACEHOLDER} objects into {@link
- * #outputArtifactData} if the filesystem supports fast digests, and the actual metadata if
- * the filesystem does not support fast digests.
- * <li>If the filesystem has fast digests <i>but</i> there is no remote execution injecting
- * metadata, then we will not store additional metadata here.
- * </ol>
- *
- * <p>Note that this means that in the vastly common cases (Google-internal, where we have fast
- * digests and remote execution, and Bazel, where there is often neither), this map is always
- * populated. Locally executed actions are the exception to this rule inside Google.
- *
- * <p>Moreover, there are some artifacts that are always stored here. First, middleman artifacts
- * have no corresponding {@link FileArtifactValue}. Second, directories' metadata contain their
- * mtimes, which the {@link FileArtifactValue} does not expose, so that has to be stored
- * separately.
- *
- * <p>Note that for files that need digests, we can't easily inject the digest in the {@link
- * FileArtifactValue} because it would complicate equality-checking on subsequent builds -- if our
- * filesystem doesn't do fast digests, the comparison value would not have a digest.
- */
- final ImmutableMap<Artifact, FileArtifactValue> getAllAdditionalOutputData() {
- return ImmutableMap.copyOf(additionalOutputData);
- }
-
/**
* Returns a set of the given tree artifact's contents.
*
@@ -150,23 +96,13 @@
}
void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) {
- // TODO(shahan): there are a couple of things that could reduce memory usage
- // 1. We might be able to skip creating an entry in `outputArtifactData` and only create
- // the `FileArtifactValue`, but there are likely downstream consumers that expect it that
- // would need to be cleaned up.
- // 2. Instead of creating an `additionalOutputData` entry, we could add the extra
- // `locationIndex` to `FileStateValue`.
injectOutputData(
output, new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex));
}
final void injectOutputData(Artifact output, FileArtifactValue artifactValue) {
injectedFiles.add(output);
-
- // While `artifactValue` carries the important information, consumers also require an entry in
- // `artifactData` so a `PLACEHOLDER` is added to `artifactData`.
- artifactData.put(output, FileArtifactValue.PLACEHOLDER);
- additionalOutputData.put(output, artifactValue);
+ artifactData.put(output, artifactValue);
}
/** Returns a set that tracks which Artifacts have had metadata injected. */
@@ -178,7 +114,6 @@
final void clear() {
artifactData.clear();
treeArtifactData.clear();
- additionalOutputData.clear();
treeArtifactContents.clear();
injectedFiles.clear();
}
@@ -187,7 +122,6 @@
final void remove(Artifact artifact) {
artifactData.remove(artifact);
treeArtifactData.remove(artifact);
- additionalOutputData.remove(artifact);
treeArtifactContents.remove(artifact);
injectedFiles.remove(artifact);
}
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 9b1e112..8a7183e 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
@@ -261,7 +261,6 @@
public void actionExecutionValueSerialization() throws Exception {
ActionLookupData dummyData = ActionLookupData.create(ALL_OWNER, 0);
Artifact.DerivedArtifact artifact1 = createDerivedArtifact("one");
- Artifact.DerivedArtifact artifact2 = createDerivedArtifact("two");
FileArtifactValue metadata1 =
ActionMetadataHandler.fileArtifactValueFromArtifact(artifact1, null, null);
SpecialArtifact treeArtifact = createDerivedTreeArtifactOnly("tree");
@@ -280,17 +279,15 @@
PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT);
ActionExecutionValue actionExecutionValue =
ActionExecutionValue.create(
- ImmutableMap.of(artifact1, metadata1, artifact2, FileArtifactValue.PLACEHOLDER),
+ ImmutableMap.of(artifact1, metadata1, artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN),
ImmutableMap.of(treeArtifact, treeArtifactValue),
- ImmutableMap.of(artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN),
ImmutableList.of(filesetOutputSymlink),
null,
true);
ActionExecutionValue valueWithFingerprint =
ActionExecutionValue.create(
- ImmutableMap.of(artifact1, metadata1, artifact2, FileArtifactValue.PLACEHOLDER),
+ ImmutableMap.of(artifact1, metadata1, artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN),
ImmutableMap.of(treeArtifact, treeArtifactValue),
- ImmutableMap.of(artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN),
ImmutableList.of(filesetOutputSymlink),
null,
true);
@@ -418,7 +415,6 @@
public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
Map<Artifact, FileArtifactValue> artifactData = new HashMap<>();
Map<Artifact, TreeArtifactValue> treeArtifactData = new HashMap<>();
- Map<Artifact, FileArtifactValue> additionalOutputData = new HashMap<>();
ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument();
ActionLookupValue actionLookupValue =
(ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey());
@@ -440,11 +436,9 @@
} else if (action.getActionType() == MiddlemanType.NORMAL) {
FileArtifactValue fileValue =
ActionMetadataHandler.fileArtifactValueFromArtifact(output, null, null);
- artifactData.put(output, fileValue);
- additionalOutputData.put(
- output, FileArtifactValue.injectDigestForTesting(output, fileValue));
+ artifactData.put(output, FileArtifactValue.injectDigestForTesting(output, fileValue));
} else {
- additionalOutputData.put(output, FileArtifactValue.DEFAULT_MIDDLEMAN);
+ artifactData.put(output, FileArtifactValue.DEFAULT_MIDDLEMAN);
}
} catch (IOException e) {
throw new IllegalStateException(e);
@@ -452,7 +446,6 @@
return ActionExecutionValue.create(
artifactData,
treeArtifactData,
- additionalOutputData,
/*outputSymlinks=*/ null,
/*discoveredModules=*/ null,
/*actionDependsOnBuildId=*/ false);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
index 4d9e29a..cb06db2 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
@@ -383,13 +383,13 @@
actionKey1,
actionValue(
new TestAction(
- Runnables.doNothing(), ImmutableSet.<Artifact>of(), ImmutableSet.of(out1)),
- forceDigests),
+ Runnables.doNothing(), ImmutableSet.<Artifact>of(), ImmutableSet.of(out1))),
actionKey2,
actionValue(
new TestAction(
- Runnables.doNothing(), ImmutableSet.<Artifact>of(), ImmutableSet.of(out2)),
- forceDigests)));
+ Runnables.doNothing(),
+ ImmutableSet.<Artifact>of(),
+ ImmutableSet.of(out2)))));
EvaluationContext evaluationContext =
EvaluationContext.newBuilder()
.setKeepGoing(false)
@@ -727,15 +727,20 @@
// TODO(bazel-team): Add some tests for FileSystemValueChecker#changedKeys*() methods.
// Presently these appear to be untested.
- private ActionExecutionValue actionValue(Action action, boolean forceDigest) {
+ private ActionExecutionValue actionValue(Action action) {
Map<Artifact, FileArtifactValue> artifactData = new HashMap<>();
for (Artifact output : action.getOutputs()) {
try {
Path path = output.getPath();
- FileStatusWithDigest stat =
- forceDigest ? statWithDigest(path, path.statIfFound(Symlinks.NOFOLLOW)) : null;
- artifactData.put(
- output, ActionMetadataHandler.fileArtifactValueFromArtifact(output, stat, null));
+ FileArtifactValue noDigest =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(
+ output,
+ FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)),
+ null);
+ FileArtifactValue withDigest =
+ FileArtifactValue.createFromInjectedDigest(
+ noDigest, path.getDigest(), !output.isConstantMetadata());
+ artifactData.put(output, withDigest);
} catch (IOException e) {
throw new IllegalStateException(e);
}
@@ -743,7 +748,6 @@
return ActionExecutionValue.create(
artifactData,
ImmutableMap.<Artifact, TreeArtifactValue>of(),
- ImmutableMap.<Artifact, FileArtifactValue>of(),
/*outputSymlinks=*/ null,
/*discoveredModules=*/ null,
/*actionDependsOnBuildId=*/ false);
@@ -756,7 +760,6 @@
return ActionExecutionValue.create(
ImmutableMap.of(),
ImmutableMap.of(emptyDir, emptyValue),
- ImmutableMap.<Artifact, FileArtifactValue>of(),
/*outputSymlinks=*/ null,
/*discoveredModules=*/ null,
/*actionDependsOnBuildId=*/ false);
@@ -774,10 +777,17 @@
dirDatum = new HashMap<>();
directoryData.put(output.getParent(), dirDatum);
}
- FileArtifactValue fileValue =
- ActionMetadataHandler.fileArtifactValueFromArtifact(output, null, null);
- dirDatum.put(output, FileArtifactValue.injectDigestForTesting(output, fileValue));
- fileData.put(output, fileValue);
+ Path path = output.getPath();
+ FileArtifactValue noDigest =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(
+ output,
+ FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)),
+ null);
+ FileArtifactValue withDigest =
+ FileArtifactValue.createFromInjectedDigest(
+ noDigest, path.getDigest(), !output.isConstantMetadata());
+ dirDatum.put(output, withDigest);
+ fileData.put(output, withDigest);
} catch (IOException e) {
throw new IllegalStateException(e);
}
@@ -792,7 +802,6 @@
return ActionExecutionValue.create(
fileData,
treeArtifactData,
- ImmutableMap.<Artifact, FileArtifactValue>of(),
/*outputSymlinks=*/ null,
/*discoveredModules=*/ null,
/*actionDependsOnBuildId=*/ false);
@@ -810,9 +819,8 @@
}
TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build());
return ActionExecutionValue.create(
- Collections.emptyMap(),
- Collections.singletonMap(output, treeArtifactValue),
ImmutableMap.of(),
+ Collections.singletonMap(output, treeArtifactValue),
/* outputSymlinks= */ null,
/* discoveredModules= */ null,
/* actionDependsOnBuildId= */ false);
@@ -821,9 +829,8 @@
private ActionExecutionValue actionValueWithRemoteArtifact(
Artifact output, RemoteFileArtifactValue value) {
return ActionExecutionValue.create(
- Collections.singletonMap(output, FileArtifactValue.PLACEHOLDER),
- ImmutableMap.of(),
Collections.singletonMap(output, value),
+ ImmutableMap.of(),
/* outputSymlinks= */ null,
/* discoveredModules= */ null,
/* actionDependsOnBuildId= */ false);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
index 76407e4..0a03985 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
@@ -41,9 +41,11 @@
import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
import com.google.devtools.build.lib.events.NullEventHandler;
import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.MemoizingEvaluator;
@@ -288,11 +290,17 @@
for (PathFragment subpath : testTreeArtifactContents) {
try {
TreeFileArtifact suboutput = ActionInputHelper.treeFileArtifact(output, subpath);
- FileArtifactValue fileValue =
- ActionMetadataHandler.fileArtifactValueFromArtifact(suboutput, null, null);
- fileData.put(suboutput, fileValue);
- treeArtifactData.put(
- suboutput, FileArtifactValue.injectDigestForTesting(suboutput, fileValue));
+ Path path = suboutput.getPath();
+ FileArtifactValue noDigest =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(
+ suboutput,
+ FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)),
+ null);
+ FileArtifactValue withDigest =
+ FileArtifactValue.createFromInjectedDigest(
+ noDigest, path.getDigest(), !output.isConstantMetadata());
+ fileData.put(suboutput, withDigest);
+ treeArtifactData.put(suboutput, withDigest);
} catch (IOException e) {
throw new SkyFunctionException(e, Transience.TRANSIENT) {};
}
@@ -303,7 +311,6 @@
return ActionExecutionValue.create(
fileData,
ImmutableMap.of(output, treeArtifactValue),
- ImmutableMap.<Artifact, FileArtifactValue>of(),
/*outputSymlinks=*/ null,
/*discoveredModules=*/ null,
/*actionDependsOnBuildId=*/ false);
