Create `RegularFileStateValueWithDigest` and `RegularFileStateValueWithContentsProxy` to replace `RegularFileStateValue` to reduce memory overhead.
The `digest` and `contentsProxy` fields are mutually exclusive in `RegularFileStateValue` class so we can refactor `RegularFileStateValue` into two classes, one of which stores `digest` and the other stores `contentsProxy`.
PiperOrigin-RevId: 516938364
Change-Id: I56d473fbbe5f117c47a8953310b0edc2267571df
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java
index ca60b04..577bd24 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java
@@ -13,11 +13,12 @@
// limitations under the License.
package com.google.devtools.build.lib.actions;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
@@ -101,8 +102,8 @@
}
return createWithStatNoFollow(
rootedPath,
- Preconditions.checkNotNull(FileStatusWithDigestAdapter.maybeAdapt(stat), rootedPath),
- /*digestWillBeInjected=*/ false,
+ checkNotNull(FileStatusWithDigestAdapter.maybeAdapt(stat), rootedPath),
+ /* digestWillBeInjected= */ false,
syscallCache,
tsgm);
}
@@ -120,7 +121,7 @@
if (statNoFollow.isFile()) {
return statNoFollow.isSpecialFile()
? SpecialFileStateValue.fromStat(path.asFragment(), statNoFollow, tsgm)
- : RegularFileStateValue.fromPath(
+ : createRegularFileStateValueFromPath(
path, statNoFollow, digestWillBeInjected, xattrProvider, tsgm);
} else if (statNoFollow.isDirectory()) {
return DIRECTORY_FILE_STATE_NODE;
@@ -131,6 +132,72 @@
+ "neither a file nor directory nor symlink.");
}
+ /**
+ * Creates a {@link FileStateValue} instance corresponding to the given existing file.
+ *
+ * <p>We use digests only if a fast digest lookup is available from the filesystem. If not, we
+ * fall back to mtime-based digests. This avoids the case where Blaze must read all files involved
+ * in the build in order to check for modifications in the case where fast digest lookups are not
+ * available.
+ *
+ * @param stat must be of type "File". (Not a symlink).
+ */
+ private static FileStateValue createRegularFileStateValueFromPath(
+ Path path,
+ FileStatusWithDigest stat,
+ boolean digestWillBeInjected,
+ XattrProvider xattrProvider,
+ @Nullable TimestampGranularityMonitor tsgm)
+ throws InconsistentFilesystemException {
+ checkState(stat.isFile(), path);
+
+ try {
+ // If the digest will be injected, we can skip calling getFastDigest, but we need to store a
+ // contents proxy because if the digest is injected but is not available from the filesystem,
+ // we will need the proxy to determine whether the file was modified.
+ byte[] digest = digestWillBeInjected ? null : tryGetDigest(path, stat, xattrProvider);
+ if (digest == null) {
+ // Note that TimestampGranularityMonitor#notifyDependenceOnFileTime is a thread-safe method.
+ if (tsgm != null) {
+ tsgm.notifyDependenceOnFileTime(path.asFragment(), stat.getLastChangeTime());
+ }
+ return new RegularFileStateValueWithContentsProxy(
+ stat.getSize(), FileContentsProxy.create(stat));
+ } else {
+ // We are careful here to avoid putting the value ID into FileMetadata if we already have a
+ // digest. Arbitrary filesystems may do weird things with the value ID; a digest is more
+ // robust.
+ return new RegularFileStateValueWithDigest(stat.getSize(), digest);
+ }
+ } catch (IOException e) {
+ String errorMessage = e.getMessage() != null ? "error '" + e.getMessage() + "'" : "an error";
+ throw new InconsistentFilesystemException(
+ "'stat' said "
+ + path
+ + " is a file but then we "
+ + "later encountered "
+ + errorMessage
+ + " which indicates that "
+ + path
+ + " is no "
+ + "longer a file. Did you delete it during the build?");
+ }
+ }
+
+ @Nullable
+ private static byte[] tryGetDigest(
+ Path path, FileStatusWithDigest stat, XattrProvider xattrProvider) throws IOException {
+ try {
+ byte[] digest = stat.getDigest();
+ return digest != null ? digest : xattrProvider.getFastDigest(path);
+ } catch (IOException ioe) {
+ if (!path.isReadable()) {
+ return null;
+ }
+ throw ioe;
+ }
+ }
+
@ThreadSafe
public static RootedPath key(RootedPath rootedPath) {
// RootedPath is already the SkyKey we want; see FileStateKey. This method and that interface
@@ -168,80 +235,89 @@
abstract String prettyPrint();
/**
- * Implementation of {@link FileStateValue} for regular files that exist.
- *
- * <p>A union of (digest, mtime). We use digests only if a fast digest lookup is available from
- * the filesystem. If not, we fall back to mtime-based digests. This avoids the case where Blaze
- * must read all files involved in the build in order to check for modifications in the case where
- * fast digest lookups are not available.
+ * Implementation of {@link FileStateValue} for regular files when a {@link #digest} is provided.
*/
- @ThreadSafe
- public static final class RegularFileStateValue extends FileStateValue {
+ public static final class RegularFileStateValueWithDigest extends FileStateValue {
private final long size;
- @Nullable private final byte[] digest;
- @Nullable private final FileContentsProxy contentsProxy;
+ private final byte[] digest;
@VisibleForTesting
- public RegularFileStateValue(long size, byte[] digest, FileContentsProxy contentsProxy) {
- Preconditions.checkState((digest == null) != (contentsProxy == null));
+ public RegularFileStateValueWithDigest(long size, byte[] digest) {
this.size = size;
- this.digest = digest;
- this.contentsProxy = contentsProxy;
+ this.digest = checkNotNull(digest);
}
- /**
- * Creates a FileFileStateValue instance corresponding to the given existing file.
- *
- * @param stat must be of type "File". (Not a symlink).
- */
- private static RegularFileStateValue fromPath(
- Path path,
- FileStatusWithDigest stat,
- boolean digestWillBeInjected,
- XattrProvider xattrProvider,
- @Nullable TimestampGranularityMonitor tsgm)
- throws InconsistentFilesystemException {
- Preconditions.checkState(stat.isFile(), path);
-
- try {
- // If the digest will be injected, we can skip calling getFastDigest, but we need to store a
- // contents proxy because if the digest is injected but is not available from the
- // filesystem, we will need the proxy to determine whether the file was modified.
- byte[] digest = digestWillBeInjected ? null : tryGetDigest(path, stat, xattrProvider);
- if (digest == null) {
- // Note that TimestampGranularityMonitor#notifyDependenceOnFileTime is a thread-safe
- // method.
- if (tsgm != null) {
- tsgm.notifyDependenceOnFileTime(path.asFragment(), stat.getLastChangeTime());
- }
- return new RegularFileStateValue(stat.getSize(), null, FileContentsProxy.create(stat));
- } else {
- // We are careful here to avoid putting the value ID into FileMetadata if we already have
- // a digest. Arbitrary filesystems may do weird things with the value ID; a digest is more
- // robust.
- return new RegularFileStateValue(stat.getSize(), digest, null);
- }
- } catch (IOException e) {
- String errorMessage = e.getMessage() != null
- ? "error '" + e.getMessage() + "'" : "an error";
- throw new InconsistentFilesystemException("'stat' said " + path + " is a file but then we "
- + "later encountered " + errorMessage + " which indicates that " + path + " is no "
- + "longer a file. Did you delete it during the build?");
- }
+ @Override
+ public FileStateType getType() {
+ return FileStateType.REGULAR_FILE;
}
+ @Override
+ public long getSize() {
+ return size;
+ }
+
+ @Override
+ public byte[] getDigest() {
+ return digest;
+ }
+
+ @Override
@Nullable
- private static byte[] tryGetDigest(
- Path path, FileStatusWithDigest stat, XattrProvider xattrProvider) throws IOException {
- try {
- byte[] digest = stat.getDigest();
- return digest != null ? digest : xattrProvider.getFastDigest(path);
- } catch (IOException ioe) {
- if (!path.isReadable()) {
- return null;
- }
- throw ioe;
+ public FileContentsProxy getContentsProxy() {
+ return null;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj == this) {
+ return true;
}
+ if (!(obj instanceof RegularFileStateValueWithDigest)) {
+ return false;
+ }
+ RegularFileStateValueWithDigest other = (RegularFileStateValueWithDigest) obj;
+ return size == other.size && Arrays.equals(digest, other.digest);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(size, Arrays.hashCode(digest));
+ }
+
+ @Override
+ public byte[] getValueFingerprint() {
+ Fingerprint fp = new Fingerprint().addLong(size);
+ fp.addBytes(digest);
+ return fp.digestAndReset();
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this).add("digest", digest).add("size", size).toString();
+ }
+
+ @Override
+ public String prettyPrint() {
+ String contents = String.format("digest of %s", Arrays.toString(digest));
+ return String.format("regular file with size of %d and %s", size, contents);
+ }
+ }
+
+ /**
+ * Implementation of {@link FileStateValue} for regular files when {@link FileContentsProxy} is
+ * provided.
+ *
+ * <p>{@link #contentsProxy} is used to determine whether the file was modified.
+ */
+ public static final class RegularFileStateValueWithContentsProxy extends FileStateValue {
+ private final long size;
+ private final FileContentsProxy contentsProxy;
+
+ @VisibleForTesting
+ public RegularFileStateValueWithContentsProxy(long size, FileContentsProxy contentsProxy) {
+ this.size = size;
+ this.contentsProxy = checkNotNull(contentsProxy);
}
@Override
@@ -257,7 +333,7 @@
@Override
@Nullable
public byte[] getDigest() {
- return digest;
+ return null;
}
@Override
@@ -270,46 +346,37 @@
if (obj == this) {
return true;
}
- if (!(obj instanceof RegularFileStateValue)) {
+ if (!(obj instanceof RegularFileStateValueWithContentsProxy)) {
return false;
}
- RegularFileStateValue other = (RegularFileStateValue) obj;
- return size == other.size
- && Arrays.equals(digest, other.digest)
- && Objects.equals(contentsProxy, other.contentsProxy);
+ RegularFileStateValueWithContentsProxy other = (RegularFileStateValueWithContentsProxy) obj;
+ return size == other.size && Objects.equals(contentsProxy, other.contentsProxy);
}
@Override
public int hashCode() {
- return Objects.hash(size, Arrays.hashCode(digest), contentsProxy);
+ return Objects.hash(size, contentsProxy);
}
@Override
public byte[] getValueFingerprint() {
Fingerprint fp = new Fingerprint().addLong(size);
- if (digest != null) {
- fp.addBytes(digest);
- }
- if (contentsProxy != null) {
- contentsProxy.addToFingerprint(fp);
- }
+ contentsProxy.addToFingerprint(fp);
return fp.digestAndReset();
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
- .add("digest", digest)
.add("size", size)
- .add("contentsProxy", contentsProxy).toString();
+ .add("contentsProxy", contentsProxy)
+ .toString();
}
@Override
public String prettyPrint() {
- String contents = digest != null
- ? String.format("digest of %s", Arrays.toString(digest))
- : contentsProxy.prettyPrint();
- return String.format("regular file with size of %d and %s", size, contents);
+ return String.format(
+ "regular file with size of %d and %s", size, contentsProxy.prettyPrint());
}
}
@@ -320,7 +387,7 @@
@VisibleForTesting
public SpecialFileStateValue(FileContentsProxy contentsProxy) {
- this.contentsProxy = Preconditions.checkNotNull(contentsProxy);
+ this.contentsProxy = checkNotNull(contentsProxy);
}
private static SpecialFileStateValue fromStat(
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 ad69283..3dc808c 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
@@ -13,10 +13,12 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
@@ -24,7 +26,8 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileStateType;
import com.google.devtools.build.lib.actions.FileStateValue;
-import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValue;
+import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithContentsProxy;
+import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithDigest;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.actions.HasDigest;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -105,14 +108,15 @@
SYMLINK_CYCLE_OR_INFINITE_EXPANSION,
}
- private final Type type;
+ private final RecursiveFilesystemTraversalException.Type type;
- RecursiveFilesystemTraversalException(String message, Type type) {
+ RecursiveFilesystemTraversalException(
+ String message, RecursiveFilesystemTraversalException.Type type) {
super(message);
this.type = type;
}
- public Type getType() {
+ public RecursiveFilesystemTraversalException.Type getType() {
return type;
}
}
@@ -129,9 +133,9 @@
super(
String.format(
"Found dangling symlink: %s, unresolved path: \"%s\"", path, unresolvedLink),
- Type.DANGLING_SYMLINK);
- Preconditions.checkArgument(path != null && !path.isEmpty());
- Preconditions.checkArgument(unresolvedLink != null && !unresolvedLink.isEmpty());
+ RecursiveFilesystemTraversalException.Type.DANGLING_SYMLINK);
+ checkArgument(path != null && !path.isEmpty());
+ checkArgument(unresolvedLink != null && !unresolvedLink.isEmpty());
}
}
@@ -288,8 +292,8 @@
HasDigest metadata,
@Nullable RootedPath realPath,
@Nullable PathFragment unresolvedSymlinkTarget) {
- Preconditions.checkNotNull(metadata.getDigest(), metadata);
- this.type = Preconditions.checkNotNull(type);
+ checkNotNull(metadata.getDigest(), metadata);
+ this.type = checkNotNull(type);
this.metadata = metadata;
this.realPath = realPath;
this.unresolvedSymlinkTarget = unresolvedSymlinkTarget;
@@ -330,7 +334,7 @@
}
RootedPath realPath = traversal.root().asRootedPath();
if (traversal.strictOutputFiles()) {
- Preconditions.checkNotNull(fsVal, "Strict Fileset output tree has null FileArtifactValue");
+ checkNotNull(fsVal, "Strict Fileset output tree has null FileArtifactValue");
return new FileInfo(
fsVal instanceof TreeArtifactValue ? FileType.DIRECTORY : FileType.FILE,
fsVal,
@@ -451,15 +455,14 @@
throws IOException {
if (fsVal instanceof FileStateValue) {
FileStateValue fsv = (FileStateValue) fsVal;
- if (fsv instanceof RegularFileStateValue) {
- RegularFileStateValue rfsv = (RegularFileStateValue) fsv;
- return rfsv.getDigest() != null
- // If we have the digest, then simply convert it with the digest value.
- ? FileArtifactValue.createForVirtualActionInput(rfsv.getDigest(), rfsv.getSize())
- // Otherwise, create a file FileArtifactValue (RegularFileArtifactValue) based on the
- // path and size.
- : FileArtifactValue.createForNormalFileUsingPath(path, rfsv.getSize(), syscallCache);
+ if (fsv instanceof RegularFileStateValueWithDigest) {
+ RegularFileStateValueWithDigest rfsv = (RegularFileStateValueWithDigest) fsv;
+ return FileArtifactValue.createForVirtualActionInput(rfsv.getDigest(), rfsv.getSize());
+ } else if (fsv instanceof RegularFileStateValueWithContentsProxy) {
+ RegularFileStateValueWithContentsProxy rfsv = (RegularFileStateValueWithContentsProxy) fsv;
+ return FileArtifactValue.createForNormalFileUsingPath(path, rfsv.getSize(), syscallCache);
}
+
return new HasDigest.ByteStringDigest(fsv.getValueFingerprint());
} else if (fsVal instanceof FileArtifactValue) {
FileArtifactValue fav = ((FileArtifactValue) fsVal);
@@ -481,37 +484,38 @@
CONFLICT, DIRECTORY, PKG
}
- private final Type type;
+ private final PkgLookupResult.Type type;
final TraversalRequest traversal;
final FileInfo rootInfo;
/** Result for a generated directory that conflicts with a source package. */
static PkgLookupResult conflict(TraversalRequest traversal, FileInfo rootInfo) {
- return new PkgLookupResult(Type.CONFLICT, traversal, rootInfo);
+ return new PkgLookupResult(PkgLookupResult.Type.CONFLICT, traversal, rootInfo);
}
/** Result for a source or generated directory (not a package). */
static PkgLookupResult directory(TraversalRequest traversal, FileInfo rootInfo) {
- return new PkgLookupResult(Type.DIRECTORY, traversal, rootInfo);
+ return new PkgLookupResult(PkgLookupResult.Type.DIRECTORY, traversal, rootInfo);
}
/** Result for a package, i.e. a directory with a BUILD file. */
static PkgLookupResult pkg(TraversalRequest traversal, FileInfo rootInfo) {
- return new PkgLookupResult(Type.PKG, traversal, rootInfo);
+ return new PkgLookupResult(PkgLookupResult.Type.PKG, traversal, rootInfo);
}
- private PkgLookupResult(Type type, TraversalRequest traversal, FileInfo rootInfo) {
- this.type = Preconditions.checkNotNull(type);
- this.traversal = Preconditions.checkNotNull(traversal);
- this.rootInfo = Preconditions.checkNotNull(rootInfo);
+ private PkgLookupResult(
+ PkgLookupResult.Type type, TraversalRequest traversal, FileInfo rootInfo) {
+ this.type = checkNotNull(type);
+ this.traversal = checkNotNull(traversal);
+ this.rootInfo = checkNotNull(rootInfo);
}
boolean isPackage() {
- return type == Type.PKG;
+ return type == PkgLookupResult.Type.PKG;
}
boolean isConflicting() {
- return type == Type.CONFLICT;
+ return type == PkgLookupResult.Type.CONFLICT;
}
@Override
@@ -531,8 +535,8 @@
private static PkgLookupResult checkIfPackage(
Environment env, TraversalRequest traversal, FileInfo rootInfo, SyscallCache syscallCache)
throws IOException, InterruptedException, BuildFileNotFoundException {
- Preconditions.checkArgument(rootInfo.type.exists() && !rootInfo.type.isFile(),
- "{%s} {%s}", traversal, rootInfo);
+ checkArgument(
+ rootInfo.type.exists() && !rootInfo.type.isFile(), "{%s} {%s}", traversal, rootInfo);
// PackageLookupFunction/dependencies can only throw IOException, BuildFileNotFoundException,
// and RepositoryFetchException, and RepositoryFetchException is not in play here. Note that
// run-of-the-mill circular symlinks will *not* throw here, and will trigger later errors during
@@ -579,8 +583,7 @@
*/
private static RecursiveFilesystemTraversalValue resultForDanglingSymlink(
RootedPath linkName, FileInfo info) {
- Preconditions.checkState(info.type.isSymlink() && !info.type.exists(), "{%s} {%s}", linkName,
- info.type);
+ checkState(info.type.isSymlink() && !info.type.exists(), "{%s} {%s}", linkName, info.type);
return RecursiveFilesystemTraversalValue.of(
ResolvedFileFactory.danglingSymlink(linkName, info.unresolvedSymlinkTarget, info.metadata));
}
@@ -593,8 +596,7 @@
*/
private static RecursiveFilesystemTraversalValue resultForFileRoot(
RootedPath path, FileInfo info) {
- Preconditions.checkState(
- info.type.isFile() && info.type.exists(), "{%s} {%s}", path, info.type);
+ checkState(info.type.isFile() && info.type.exists(), "{%s} {%s}", path, info.type);
if (info.type.isSymlink()) {
return RecursiveFilesystemTraversalValue.of(
ResolvedFileFactory.symlinkToFile(
