Skip unnecessary extra attempts to get fast digests.
The extra condition for tree artifact children was left over from previous code versions but is no longer necessary.
Also, when there is no injected digest and no fast digest, we can skip the fast digest attempt and go straight to a slow digest.
PiperOrigin-RevId: 319983014
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 27a205f..777e468 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
@@ -246,7 +246,7 @@
return new DirectoryArtifactValue(path.getLastModifiedTime());
}
if (digest == null) {
- digest = DigestUtils.getDigestOrFail(path, size);
+ digest = DigestUtils.getDigestWithManualFallback(path, size);
}
Preconditions.checkState(digest != null, path);
return createForNormalFile(digest, proxy, size, isShareable);
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java b/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
index 153c570..8539846 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
@@ -213,20 +213,32 @@
}
/**
- * Get the digest of {@code path}, using a constant-time xattr call if the filesystem supports
+ * Gets the digest of {@code path}, using a constant-time xattr call if the filesystem supports
* it, and calculating the digest manually otherwise.
*
+ * <p>If {@link Path#getFastDigest} has already been attempted and was not available, call {@link
+ * #manuallyComputeDigest} to skip an additional attempt to obtain the fast digest.
+ *
* @param path Path of the file.
- * @param fileSize size of the file. Used to determine if digest calculation should be done
- * serially or in parallel. Files larger than a certain threshold will be read serially, in order
- * to avoid excessive disk seeks.
+ * @param fileSize Size of the file. Used to determine if digest calculation should be done
+ * serially or in parallel. Files larger than a certain threshold will be read serially, in
+ * order to avoid excessive disk seeks.
*/
- public static byte[] getDigestOrFail(Path path, long fileSize)
- throws IOException {
+ public static byte[] getDigestWithManualFallback(Path path, long fileSize) throws IOException {
byte[] digest = path.getFastDigest();
- if (digest != null) {
- return digest;
- }
+ return digest != null ? digest : manuallyComputeDigest(path, fileSize);
+ }
+
+ /**
+ * Calculates the digest manually.
+ *
+ * @param path Path of the file.
+ * @param fileSize Size of the file. Used to determine if digest calculation should be done
+ * serially or in parallel. Files larger than a certain threshold will be read serially, in
+ * order to avoid excessive disk seeks.
+ */
+ public static byte[] manuallyComputeDigest(Path path, long fileSize) throws IOException {
+ byte[] digest;
// Attempt a cache lookup if the cache is enabled.
Cache<CacheKey, byte[]> cache = globalCache;
@@ -250,7 +262,7 @@
digest = getDigestInternal(path);
}
- Preconditions.checkNotNull(digest);
+ Preconditions.checkNotNull(digest, "Missing digest for %s (size %s)", path, fileSize);
if (cache != null) {
cache.put(key, digest);
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
index ee3f06d..1b294af 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
@@ -61,7 +61,7 @@
}
public Digest compute(Path file, long fileSize) throws IOException {
- return buildDigest(DigestUtils.getDigestOrFail(file, fileSize), fileSize);
+ return buildDigest(DigestUtils.getDigestWithManualFallback(file, fileSize), fileSize);
}
public Digest compute(VirtualActionInput input) throws IOException {
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 bd20553..1ff255c 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
@@ -523,9 +523,8 @@
return value;
}
- if (type.isFile() && !artifact.hasParent() && fileDigest != null) {
- // We do not need to store the FileArtifactValue separately -- the digest is in the file value
- // and that is all that is needed for this file's metadata.
+ if (type.isFile() && fileDigest != null) {
+ // The digest is in the file value and that is all that is needed for this file's metadata.
return value;
}
@@ -541,8 +540,10 @@
}
if (injectedDigest == null && type.isFile()) {
+ // We don't have an injected digest and there is no digest in the file value (which attempts a
+ // fast digest). Manually compute the digest instead.
injectedDigest =
- DigestUtils.getDigestOrFail(artifactPathResolver.toPath(artifact), value.getSize());
+ DigestUtils.manuallyComputeDigest(artifactPathResolver.toPath(artifact), value.getSize());
}
return FileArtifactValue.createFromInjectedDigest(
value, injectedDigest, !artifact.isConstantMetadata());
diff --git a/src/test/java/com/google/devtools/build/lib/actions/DigestUtilsTest.java b/src/test/java/com/google/devtools/build/lib/actions/DigestUtilsTest.java
index c74c41c..eb1ae6c 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/DigestUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/DigestUtilsTest.java
@@ -85,8 +85,10 @@
FileSystemUtils.writeContentAsLatin1(myFile1, Strings.repeat("a", fileSize1));
FileSystemUtils.writeContentAsLatin1(myFile2, Strings.repeat("b", fileSize2));
- TestThread thread1 = new TestThread(() -> DigestUtils.getDigestOrFail(myFile1, fileSize1));
- TestThread thread2 = new TestThread(() -> DigestUtils.getDigestOrFail(myFile2, fileSize2));
+ TestThread thread1 =
+ new TestThread(() -> DigestUtils.getDigestWithManualFallback(myFile1, fileSize1));
+ TestThread thread2 =
+ new TestThread(() -> DigestUtils.getDigestWithManualFallback(myFile2, fileSize2));
thread1.start();
thread2.start();
if (!expectConcurrent) { // Synchronized case.
@@ -151,7 +153,7 @@
return this;
}
- void check() throws Exception {
+ void check() {
assertThat(stats.evictionCount()).isEqualTo(expectedEvictionCount);
assertThat(stats.hitCount()).isEqualTo(expectedHitCount);
assertThat(stats.missCount()).isEqualTo(expectedMissCount);
@@ -166,7 +168,7 @@
FileSystem tracingFileSystem =
new InMemoryFileSystem(BlazeClock.instance()) {
@Override
- protected byte[] getFastDigest(Path path) throws IOException {
+ protected byte[] getFastDigest(Path path) {
getFastDigestCounter.incrementAndGet();
return null;
}
@@ -187,12 +189,12 @@
FileSystemUtils.writeContentAsLatin1(file2, "some other contents");
FileSystemUtils.writeContentAsLatin1(file3, "and something else");
- byte[] digest1 = DigestUtils.getDigestOrFail(file1, file1.getFileSize());
+ byte[] digest1 = DigestUtils.getDigestWithManualFallback(file1, file1.getFileSize());
assertThat(getFastDigestCounter.get()).isEqualTo(1);
assertThat(getDigestCounter.get()).isEqualTo(1);
new CacheStatsChecker().evictionCount(0).hitCount(0).missCount(1).check();
- byte[] digest2 = DigestUtils.getDigestOrFail(file1, file1.getFileSize());
+ byte[] digest2 = DigestUtils.getDigestWithManualFallback(file1, file1.getFileSize());
assertThat(getFastDigestCounter.get()).isEqualTo(2);
assertThat(getDigestCounter.get()).isEqualTo(1);
new CacheStatsChecker().evictionCount(0).hitCount(1).missCount(1).check();
@@ -200,14 +202,35 @@
assertThat(digest2).isEqualTo(digest1);
// Evict the digest for the previous file.
- DigestUtils.getDigestOrFail(file2, file2.getFileSize());
- DigestUtils.getDigestOrFail(file3, file3.getFileSize());
+ DigestUtils.getDigestWithManualFallback(file2, file2.getFileSize());
+ DigestUtils.getDigestWithManualFallback(file3, file3.getFileSize());
new CacheStatsChecker().evictionCount(1).hitCount(1).missCount(3).check();
// And now try to recompute it.
- byte[] digest3 = DigestUtils.getDigestOrFail(file1, file1.getFileSize());
+ byte[] digest3 = DigestUtils.getDigestWithManualFallback(file1, file1.getFileSize());
new CacheStatsChecker().evictionCount(2).hitCount(1).missCount(4).check();
assertThat(digest3).isEqualTo(digest1);
}
+
+ @Test
+ public void manuallyComputeDigest() throws Exception {
+ byte[] digest = {1, 2, 3};
+ FileSystem noDigestFileSystem =
+ new InMemoryFileSystem(BlazeClock.instance()) {
+ @Override
+ protected byte[] getFastDigest(Path path) {
+ throw new AssertionError("Unexpected call to getFastDigest");
+ }
+
+ @Override
+ protected byte[] getDigest(Path path) {
+ return digest;
+ }
+ };
+ Path file = noDigestFileSystem.getPath("/f.txt");
+ FileSystemUtils.writeContentAsLatin1(file, "contents");
+
+ assertThat(DigestUtils.manuallyComputeDigest(file, /*fileSize=*/ 8)).isEqualTo(digest);
+ }
}
