Skip to content

Commit

Permalink
Remove unnecessary synchronization on RepositoryCache
Browse files Browse the repository at this point in the history 
With a single exception that didn't matter for correctness, the operations on `RepositoryCache` were already atomic on the level of the file system and thus don't require `synchronized`. In fact, since different Bazel server instances share the same repository cache concurrently, this already had to be the case.

The exception was a file write that stores the canonical id in a file whose name contains a hash of the id. However, the content of this file is never read, only its existence matters. The write is replaced by a touch.

Also removes unnecessary interrupt checks and method overloads only used in tests.

Work towards #20369

Closes #21403.

PiperOrigin-RevId: 614728666
Change-Id: I67f81bd9e468260e4f83f15d6aaafa57e34d18f4
  • Loading branch information
@fmeum @copybara-github
fmeum authored and copybara-github committed Mar 11, 2024
1 parent a235199 commit 8eade04
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 46 deletions.
47 changes: 11 additions & 36 deletions src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@
import com.google.devtools.build.lib.vfs.Path;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.UUID;
import javax.annotation.Nullable;

/** The cache implementation to store download artifacts from external repositories.
* TODO(jingwen): Implement file locking for concurrent cache accesses.
/**
* The cache implementation to store download artifacts from external repositories.
*
* <p>Operations performed by this class are atomic on the file system level under the assumption
* that the cache directory is not subject to concurrent file deletion.
*/
public class RepositoryCache {

Expand Down Expand Up @@ -134,11 +136,6 @@ boolean hasCanonicalId(String cacheKey, KeyType keyType, String canonicalId) {
.exists();
}

public synchronized Path get(String cacheKey, Path targetPath, KeyType keyType)
throws IOException, InterruptedException {
return get(cacheKey, targetPath, keyType, null);
}

/**
* Copy or hardlink cached value to a specified directory, if it exists.
*
Expand All @@ -156,12 +153,8 @@ public synchronized Path get(String cacheKey, Path targetPath, KeyType keyType)
* @throws IOException
*/
@Nullable
public synchronized Path get(
String cacheKey, Path targetPath, KeyType keyType, String canonicalId)
public Path get(String cacheKey, Path targetPath, KeyType keyType, String canonicalId)
throws IOException, InterruptedException {
if (Thread.interrupted()) {
throw new InterruptedException();
}
Preconditions.checkState(isEnabled());

assertKeyIsValid(cacheKey, keyType);
Expand Down Expand Up @@ -202,11 +195,6 @@ public synchronized Path get(
return targetPath;
}

public synchronized void put(String cacheKey, Path sourcePath, KeyType keyType)
throws IOException, InterruptedException {
put(cacheKey, sourcePath, keyType, null);
}

/**
* Copies a value from a specified path into the cache.
*
Expand All @@ -217,13 +205,8 @@ public synchronized void put(String cacheKey, Path sourcePath, KeyType keyType)
* restricted cache lookups later.
* @throws IOException
*/
public synchronized void put(
String cacheKey, Path sourcePath, KeyType keyType, String canonicalId)
throws IOException, InterruptedException {
// Check for interrupts while waiting for the monitor of this synchronized method
if (Thread.interrupted()) {
throw new InterruptedException();
}
public void put(String cacheKey, Path sourcePath, KeyType keyType, String canonicalId)
throws IOException {
Preconditions.checkState(isEnabled());

assertKeyIsValid(cacheKey, keyType);
Expand All @@ -237,19 +220,11 @@ public synchronized void put(
FileSystemUtils.moveFile(tmpName, cacheValue);

if (!Strings.isNullOrEmpty(canonicalId)) {
byte[] canonicalIdBytes = canonicalId.getBytes(UTF_8);
String idHash = keyType.newHasher().putBytes(canonicalIdBytes).hash().toString();
OutputStream idStream = cacheEntry.getRelative(ID_PREFIX + idHash).getOutputStream();
idStream.write(canonicalIdBytes);
idStream.close();
String idHash = keyType.newHasher().putBytes(canonicalId.getBytes(UTF_8)).hash().toString();
FileSystemUtils.touchFile(cacheEntry.getRelative(ID_PREFIX + idHash));
}
}

public synchronized String put(Path sourcePath, KeyType keyType)
throws IOException, InterruptedException {
return put(sourcePath, keyType, null);
}

/**
* Copies a value from a specified path into the cache, computing the cache key itself.
*
Expand All @@ -260,7 +235,7 @@ public synchronized String put(Path sourcePath, KeyType keyType)
* @throws IOException
* @return The key for the cached entry.
*/
public synchronized String put(Path sourcePath, KeyType keyType, String canonicalId)
public String put(Path sourcePath, KeyType keyType, String canonicalId)
throws IOException, InterruptedException {
String cacheKey = getChecksum(keyType, sourcePath);
put(cacheKey, sourcePath, keyType, canonicalId);
Expand Down
30 changes: 20 additions & 10 deletions src/test/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ public void testNonExistentCacheValue() {
/** Test that the put method correctly stores the downloaded file into the cache. */
@Test
public void testPutCacheValue() throws Exception {
repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256);
repositoryCache.put(
downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null);

Path cacheEntry = KeyType.SHA256.getCachePath(contentAddressableCachePath).getChild(downloadedFileSha256);
Path cacheValue = cacheEntry.getChild(RepositoryCache.DEFAULT_CACHE_FILENAME);
Expand All @@ -88,7 +89,7 @@ public void testPutCacheValue() throws Exception {
*/
@Test
public void testPutCacheValueWithoutHash() throws Exception {
String cacheKey = repositoryCache.put(downloadedFile, KeyType.SHA256);
String cacheKey = repositoryCache.put(downloadedFile, KeyType.SHA256, /* canonicalId= */ null);
assertThat(cacheKey).isEqualTo(downloadedFileSha256);

Path cacheEntry =
Expand All @@ -105,8 +106,10 @@ public void testPutCacheValueWithoutHash() throws Exception {
*/
@Test
public void testPutCacheValueIdempotent() throws Exception {
repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256);
repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256);
repositoryCache.put(
downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null);
repositoryCache.put(
downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null);

Path cacheEntry = KeyType.SHA256.getCachePath(contentAddressableCachePath).getChild(downloadedFileSha256);
Path cacheValue = cacheEntry.getChild(RepositoryCache.DEFAULT_CACHE_FILENAME);
Expand All @@ -119,11 +122,14 @@ public void testPutCacheValueIdempotent() throws Exception {
@Test
public void testGetCacheValue() throws Exception {
// Inject file into cache
repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256);
repositoryCache.put(
downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null);

Path targetDirectory = scratch.dir("/external");
Path targetPath = targetDirectory.getChild(downloadedFile.getBaseName());
Path actualTargetPath = repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256);
Path actualTargetPath =
repositoryCache.get(
downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null);

// Check that the contents are the same.
assertThat(FileSystemUtils.readContent(downloadedFile, Charset.defaultCharset()))
Expand All @@ -138,7 +144,9 @@ public void testGetCacheValue() throws Exception {
public void testGetNullCacheValue() throws Exception {
Path targetDirectory = scratch.dir("/external");
Path targetPath = targetDirectory.getChild(downloadedFile.getBaseName());
Path actualTargetPath = repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256);
Path actualTargetPath =
repositoryCache.get(
downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null);

assertThat(actualTargetPath).isNull();
}
Expand All @@ -148,7 +156,7 @@ public void testInvalidSha256Throws() throws Exception {
String invalidSha = "foo";
thrown.expect(IOException.class);
thrown.expectMessage("Invalid key \"foo\" of type SHA-256");
repositoryCache.put(invalidSha, downloadedFile, KeyType.SHA256);
repositoryCache.put(invalidSha, downloadedFile, KeyType.SHA256, /* canonicalId= */ null);
}

@Test
Expand All @@ -165,7 +173,7 @@ public void testPoisonedCache() throws Exception {
thrown.expectMessage("does not match expected");
thrown.expectMessage("Please delete the directory");

repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256);
repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null);
}

@Test
Expand Down Expand Up @@ -215,7 +223,9 @@ public void testCanonicalId() throws Exception {
repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256, "barid");
assertThat(lookupOtherId).isNull();

Path lookupNoId = repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256);
Path lookupNoId =
repositoryCache.get(
downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null);
assertThat(lookupNoId).isEqualTo(targetPath);
}
}

0 comments on commit 8eade04

Please sign in to comment.