Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Remove the fileSize parameter from DigestUtils. #21328

Merged
merged 1 commit into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -176,8 +176,7 @@ static 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);
}
}
Loading