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.

Sign up for GitHub

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

Remove unnecessary synchronization on RepositoryCache #21403

Closed
wants to merge 1 commit into from
Closed

Remove unnecessary synchronization on RepositoryCache #21403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 11 additions & 36 deletions src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java
View file
Open in desktop
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));
Wyverald marked this conversation as resolved.
Show resolved Hide resolved
}
}

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
Open in desktop
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);
}
}
Loading