Skip to content

Commit

Permalink
[7.1.0] Remove the fileSize parameter from DigestUtils. (#21328)
Browse files Browse the repository at this point in the history
The parameter is claimed to affect whether the file to be digested is
read serially or in parallel. However, it's never actually used; that
logic appears to have been removed in unknown commit.

PiperOrigin-RevId: 606592076
Change-Id: Ia36ec3a05553cccf014d5ebb3d45b7b3c1ef83b6
  • Loading branch information
tjgq authored Feb 13, 2024
1 parent 117050c commit 9bc0c63
Show file tree
Hide file tree
Showing 9 changed files with 23 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ private static FileArtifactValue create(
return new DirectoryArtifactValue(path.getLastModifiedTime());
}
if (digest == null) {
digest = DigestUtils.getDigestWithManualFallback(path, size, xattrProvider);
digest = DigestUtils.getDigestWithManualFallback(path, xattrProvider);
}
Preconditions.checkState(digest != null, path);
return createForNormalFile(digest, proxy, size);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,8 @@ private void updateRunfilesTree(
// an up-to-date check.
if (!outputManifest.isSymbolicLink()
&& Arrays.equals(
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest, xattrProvider),
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(
inputManifest, xattrProvider))) {
DigestUtils.getDigestWithManualFallback(outputManifest, xattrProvider),
DigestUtils.getDigestWithManualFallback(inputManifest, xattrProvider))) {
return;
}
} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,7 @@ protected Digest computeDigest(
// Try to obtain a digest from the filesystem.
return builder
.setHash(
HashCode.fromBytes(
DigestUtils.getDigestWithManualFallback(path, fileSize, xattrProvider))
HashCode.fromBytes(DigestUtils.getDigestWithManualFallback(path, xattrProvider))
.toString())
.setSizeBytes(fileSize)
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ public Digest compute(Path file) throws IOException {
}

public Digest compute(Path file, long fileSize) throws IOException {
return buildDigest(
DigestUtils.getDigestWithManualFallback(file, fileSize, xattrProvider), fileSize);
return buildDigest(DigestUtils.getDigestWithManualFallback(file, xattrProvider), fileSize);
}

public Digest compute(VirtualActionInput input) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ private FileArtifactValue constructFileArtifactValue(
// possible to hit the digest cache - we probably already computed the digest for the
// target during previous action execution.
Path pathToDigest = isResolvedSymlink ? statAndValue.realPath() : statAndValue.pathNoFollow();
actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest, value.getSize());
actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest);
}

if (!isResolvedSymlink) {
Expand Down
31 changes: 5 additions & 26 deletions src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -150,40 +150,19 @@ public static CacheStats getCacheStats() {
* #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.
*/
public static byte[] getDigestWithManualFallback(
Path path, long fileSize, XattrProvider xattrProvider) throws IOException {
public static byte[] getDigestWithManualFallback(Path path, XattrProvider xattrProvider)
throws IOException {
byte[] digest = xattrProvider.getFastDigest(path);
return digest != null ? digest : manuallyComputeDigest(path, fileSize);
}

/**
* Gets the digest of {@code path}, using a constant-time xattr call if the filesystem supports
* it, and calculating the digest manually otherwise.
*
* <p>Unlike {@link #getDigestWithManualFallback}, will not rate-limit manual digesting of files,
* so only use this method if the file size is truly unknown and you don't expect many concurrent
* manual digests of large files.
*
* @param path Path of the file.
*/
public static byte[] getDigestWithManualFallbackWhenSizeUnknown(
Path path, XattrProvider xattrProvider) throws IOException {
return getDigestWithManualFallback(path, -1, xattrProvider);
return digest != null ? digest : manuallyComputeDigest(path);
}

/**
* 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 {
public static byte[] manuallyComputeDigest(Path path) throws IOException {
byte[] digest;

// Attempt a cache lookup if the cache is enabled.
Expand All @@ -199,7 +178,7 @@ public static byte[] manuallyComputeDigest(Path path, long fileSize) throws IOEx

digest = path.getDigest();

Preconditions.checkNotNull(digest, "Missing digest for %s (size %s)", path, fileSize);
Preconditions.checkNotNull(digest, "Missing digest for %s", path);
if (cache != null) {
cache.put(key, digest);
}
Expand Down
6 changes: 2 additions & 4 deletions src/main/java/com/google/devtools/build/lib/vfs/Path.java
Original file line number Diff line number Diff line change
Expand Up @@ -717,8 +717,7 @@ public String getDirectoryDigest(XattrProvider xattrProvider) throws IOException
} else {
hasher.putChar('-');
}
hasher.putBytes(
DigestUtils.getDigestWithManualFallback(path, stat.getSize(), xattrProvider));
hasher.putBytes(DigestUtils.getDigestWithManualFallback(path, xattrProvider));
} else if (stat.isDirectory()) {
hasher.putChar('d').putUnencodedChars(path.getDirectoryDigest(xattrProvider));
} else if (stat.isSymbolicLink()) {
Expand All @@ -732,8 +731,7 @@ public String getDirectoryDigest(XattrProvider xattrProvider) throws IOException
} else {
hasher.putChar('-');
}
hasher.putBytes(
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(resolved, xattrProvider));
hasher.putBytes(DigestUtils.getDigestWithManualFallback(resolved, xattrProvider));
} else {
// link to a non-file: include the link itself in the hash
hasher.putChar('l').putUnencodedChars(link.toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,7 @@ public void testFoo() throws Exception {
// Instead of actually trying to access this file, a call to getxattr() should be made. We
// intercept this call and return a fake extended attribute value, thereby causing the checksum
// computation to be skipped entirely.
assertThat(
DigestUtils.getDigestWithManualFallback(
absolutize("myfile"), 123, SyscallCache.NO_CACHE))
assertThat(DigestUtils.getDigestWithManualFallback(absolutize("myfile"), SyscallCache.NO_CACHE))
.isEqualTo(FAKE_DIGEST);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,14 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {

TestThread thread1 =
new TestThread(
() ->
DigestUtils.getDigestWithManualFallback(myFile1, fileSize1, SyscallCache.NO_CACHE));
() -> {
var unused = DigestUtils.getDigestWithManualFallback(myFile1, SyscallCache.NO_CACHE);
});
TestThread thread2 =
new TestThread(
() ->
DigestUtils.getDigestWithManualFallback(myFile2, fileSize2, SyscallCache.NO_CACHE));
() -> {
var unused = DigestUtils.getDigestWithManualFallback(myFile2, SyscallCache.NO_CACHE);
});
thread1.start();
thread2.start();
if (!expectConcurrent) { // Synchronized case.
Expand Down Expand Up @@ -120,14 +122,11 @@ protected byte[] getDigest(PathFragment path) throws IOException {
Path file = tracingFileSystem.getPath("/file.txt");
FileSystemUtils.writeContentAsLatin1(file, "some contents");

byte[] digest1 =
DigestUtils.getDigestWithManualFallback(file, file.getFileSize(), SyscallCache.NO_CACHE);
byte[] digest1 = DigestUtils.getDigestWithManualFallback(file, SyscallCache.NO_CACHE);
assertThat(getFastDigestCounter.get()).isEqualTo(1);
assertThat(getDigestCounter.get()).isEqualTo(1);

assertThat(
DigestUtils.getDigestWithManualFallback(
file, file.getFileSize(), SyscallCache.NO_CACHE))
assertThat(DigestUtils.getDigestWithManualFallback(file, SyscallCache.NO_CACHE))
.isEqualTo(digest1);
assertThat(getFastDigestCounter.get()).isEqualTo(2);
assertThat(getDigestCounter.get()).isEqualTo(1); // Cached.
Expand All @@ -151,6 +150,6 @@ protected byte[] getDigest(PathFragment path) {
Path file = noDigestFileSystem.getPath("/f.txt");
FileSystemUtils.writeContentAsLatin1(file, "contents");

assertThat(DigestUtils.manuallyComputeDigest(file, /*fileSize=*/ 8)).isEqualTo(digest);
assertThat(DigestUtils.manuallyComputeDigest(file)).isEqualTo(digest);
}
}

0 comments on commit 9bc0c63

Please sign in to comment.