Skip to content

Commit

Permalink
Remove DigestUtils.getDigestInExclusiveMode() now that SsdModule has …
Browse files Browse the repository at this point in the history
…been removed

There used to be a cli flag --experimental_multi_threaded_digest which defaulted to true
and therefore would not compute the file digest in exclusive mode unless overridden to false.
When that flag was removed this code was accidentally left behind which instead causes bazel to always
compute file digests in exclusive mode for all files larger than MULTI_THREADED_DIGEST_MAX_FILE_SIZE.

This causes bazel to take 5-6x longer to 'check cached actions' in my org's builds,
specifically increasing the time from 30 secs to 2 mins 30 secs for fully cached no-op builds.

Fixes #14034

Closes #14231.

PiperOrigin-RevId: 407790990
  • Loading branch information
Brandon Jacklyn authored and copybara-github committed Nov 5, 2021
1 parent 917e15e commit 77a002c
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 54 deletions.
35 changes: 1 addition & 34 deletions src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,8 @@
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.stats.CacheStats;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.primitives.Longs;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import java.io.IOException;

/**
Expand All @@ -41,17 +38,9 @@
* fail.
*/
public class DigestUtils {
// Object to synchronize on when serializing large file reads.
private static final Object DIGEST_LOCK = new Object();

// Typical size for a digest byte array.
public static final int ESTIMATED_SIZE = 32;

// Files of this size or less are assumed to be readable in one seek.
// (This is the default readahead window on Linux.)
@VisibleForTesting // the unittest is in a different package!
public static final int MULTI_THREADED_DIGEST_MAX_FILE_SIZE = 128 * 1024;

/**
* Keys used to cache the values of the digests for files where we don't have fast digests.
*
Expand Down Expand Up @@ -126,19 +115,6 @@ public int hashCode() {
/** Private constructor to prevent instantiation of utility class. */
private DigestUtils() {}

/**
* Obtain file's digset using synchronized method, ensuring that system is not overloaded in case
* when multiple threads are requesting digest calculations and underlying file system cannot
* provide it via extended attribute.
*/
private static byte[] getDigestInExclusiveMode(Path path) throws IOException {
long startTime = Profiler.nanoTimeMaybe();
synchronized (DIGEST_LOCK) {
Profiler.instance().logSimpleTask(startTime, ProfilerTask.WAIT, path.getPathString());
return path.getDigest();
}
}

/**
* Enables the caching of file digests based on file status data.
*
Expand Down Expand Up @@ -221,16 +197,7 @@ public static byte[] manuallyComputeDigest(Path path, long fileSize) throws IOEx
}
}

// Compute digest from the file contents.
if (fileSize > MULTI_THREADED_DIGEST_MAX_FILE_SIZE) {
// We'll have to read file content in order to calculate the digest.
// We avoid overlapping this process for multiple large files, as
// seeking back and forth between them will result in an overall loss of
// throughput.
digest = getDigestInExclusiveMode(path);
} else {
digest = path.getDigest();
}
digest = path.getDigest();

Preconditions.checkNotNull(digest, "Missing digest for %s (size %s)", path, fileSize);
if (cache != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import java.io.IOException;
import java.util.Arrays;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
Expand Down Expand Up @@ -92,25 +91,6 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
thread2.joinAndAssertState(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
}

/**
* Ensures that digest calculation is synchronized for files greater than
* {@link DigestUtils#MULTI_THREADED_DIGEST_MAX_FILE_SIZE} bytes if the digest is not
* available cheaply, so machines with rotating drives don't become unusable.
*/
@Test
public void testCalculationConcurrency() throws Exception {
int small = DigestUtils.MULTI_THREADED_DIGEST_MAX_FILE_SIZE;
int large = DigestUtils.MULTI_THREADED_DIGEST_MAX_FILE_SIZE + 1;
for (DigestHashFunction hf :
Arrays.asList(DigestHashFunction.SHA256, DigestHashFunction.SHA1)) {
assertDigestCalculationConcurrency(true, true, small, small, hf);
assertDigestCalculationConcurrency(true, true, large, large, hf);
assertDigestCalculationConcurrency(true, false, small, small, hf);
assertDigestCalculationConcurrency(true, false, small, large, hf);
assertDigestCalculationConcurrency(false, false, large, large, hf);
}
}

@Test
public void testCache() throws Exception {
AtomicInteger getFastDigestCounter = new AtomicInteger(0);
Expand Down

0 comments on commit 77a002c

Please sign in to comment.