Skip to content

Commit

Permalink
Do the AC integrity check for disk part of the combined cache.
Browse files Browse the repository at this point in the history 
This will decrease the cache-hit rate for disk cache when building without the bytes. See bazelbuild#17080 for reasoning.

With lease service, the decrement will be fixed.

Fixes bazelbuild#17080.

Closes bazelbuild#17201.

PiperOrigin-RevId: 502818641
Change-Id: I1f73dd38d7e52e2f39b367e6114aab714de22d3c
  • Loading branch information
@coeuvre @copybara-github
coeuvre authored and copybara-github committed Jan 18, 2023
1 parent 928e0fe commit 5f2866f
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 30 deletions.
60 changes: 41 additions & 19 deletions src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,31 +183,53 @@ public ListenableFuture<Void> downloadBlob(
}
}

private ListenableFuture<CachedActionResult> downloadActionResultFromRemote(
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
return Futures.transformAsync(
remoteCache.downloadActionResult(context, actionKey, inlineOutErr),
(cachedActionResult) -> {
if (cachedActionResult == null) {
return immediateFuture(null);
} else {
return Futures.transform(
diskCache.uploadActionResult(context, actionKey, cachedActionResult.actionResult()),
v -> cachedActionResult,
directExecutor());
}
},
directExecutor());
}

@Override
public ListenableFuture<CachedActionResult> downloadActionResult(
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
if (context.getReadCachePolicy().allowDiskCache()
&& diskCache.containsActionResult(actionKey)) {
return diskCache.downloadActionResult(context, actionKey, inlineOutErr);
ListenableFuture<CachedActionResult> future = immediateFuture(null);

if (context.getReadCachePolicy().allowDiskCache()) {
// If Build without the Bytes is enabled, the future will likely return null
// and fallback to remote cache because AC integrity check is enabled and referenced blobs are
// probably missing from disk cache due to BwoB.
//
// TODO(chiwang): With lease service, instead of doing the integrity check against local
// filesystem, we can check whether referenced blobs are alive in the lease service to
// increase the cache-hit rate for disk cache.
future = diskCache.downloadActionResult(context, actionKey, inlineOutErr);
}

if (context.getReadCachePolicy().allowRemoteCache()) {
return Futures.transformAsync(
remoteCache.downloadActionResult(context, actionKey, inlineOutErr),
(cachedActionResult) -> {
if (cachedActionResult == null) {
return immediateFuture(null);
} else {
return Futures.transform(
diskCache.uploadActionResult(
context, actionKey, cachedActionResult.actionResult()),
v -> cachedActionResult,
directExecutor());
}
},
directExecutor());
} else {
return immediateFuture(null);
future =
Futures.transformAsync(
future,
(result) -> {
if (result == null) {
return downloadActionResultFromRemote(context, actionKey, inlineOutErr);
} else {
return immediateFuture(result);
}
},
directExecutor());
}

return future;
}
}
11 changes: 5 additions & 6 deletions src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,6 @@ public boolean contains(Digest digest) {
return toPath(digest.getHash(), /* actionResult= */ false).exists();
}

/** Returns {@code true} if the provided {@code key} is stored in the Action Cache. */
public boolean containsActionResult(ActionKey actionKey) {
return toPath(actionKey.getDigest().getHash(), /* actionResult= */ true).exists();
}

public void captureFile(Path src, Digest digest, boolean isActionCache) throws IOException {
Path target = toPath(digest.getHash(), isActionCache);
target.getParentDirectory().createDirectoryAndParents();
Expand Down Expand Up @@ -161,7 +156,11 @@ public ListenableFuture<CachedActionResult> downloadActionResult(
}

if (checkActionResult) {
checkActionResult(actionResult);
try {
checkActionResult(actionResult);
} catch (CacheNotFoundException e) {
return Futures.immediateFuture(null);
}
}

return Futures.immediateFuture(CachedActionResult.disk(actionResult));
Expand Down
5 changes: 5 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,15 +171,20 @@ java_test(
java_test(
name = "DiskCacheIntegrationTest",
srcs = ["DiskCacheIntegrationTest.java"],
runtime_deps = [
"//third_party/grpc-java:grpc-jar",
],
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module",
"//src/main/java/com/google/devtools/build/lib/remote",
"//src/main/java/com/google/devtools/build/lib/standalone",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/test/java/com/google/devtools/build/lib/buildtool/util",
"//src/test/java/com/google/devtools/build/lib/remote/util:integration_test_utils",
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
],
)
95 changes: 90 additions & 5 deletions src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,14 @@
// limitations under the License.
package com.google.devtools.build.lib.remote;

import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.testutil.TestUtils.tmpDirFile;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule;
import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
import com.google.devtools.build.lib.remote.util.IntegrationTestUtils;
import com.google.devtools.build.lib.remote.util.IntegrationTestUtils.WorkerInstance;
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.BlazeRuntime;
import com.google.devtools.build.lib.runtime.BlockWaitingModule;
Expand All @@ -32,6 +35,25 @@

@RunWith(JUnit4.class)
public class DiskCacheIntegrationTest extends BuildIntegrationTestCase {
private WorkerInstance worker;

private void startWorker() throws Exception {
if (worker == null) {
worker = IntegrationTestUtils.startWorker();
}
}

private void enableRemoteExec(String... additionalOptions) {
assertThat(worker).isNotNull();
addOptions("--remote_executor=grpc://localhost:" + worker.getPort());
addOptions(additionalOptions);
}

private void enableRemoteCache(String... additionalOptions) {
assertThat(worker).isNotNull();
addOptions("--remote_cache=grpc://localhost:" + worker.getPort());
addOptions(additionalOptions);
}

private static PathFragment getDiskCacheDir() {
PathFragment testTmpDir = PathFragment.create(tmpDirFile().getAbsolutePath());
Expand All @@ -48,6 +70,10 @@ protected void setupOptions() throws Exception {
@After
public void tearDown() throws IOException {
getWorkspace().getFileSystem().getPath(getDiskCacheDir()).deleteTree();

if (worker != null) {
worker.stop();
}
}

@Override
Expand Down Expand Up @@ -93,7 +119,7 @@ public void hitDiskCache() throws Exception {
events.assertContainsInfo("2 disk cache hit");
}

private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOptions)
private void doBlobsReferencedInAcAreMissingFromCasIgnoresAc(String... additionalOptions)
throws Exception {
// Arrange: Prepare the workspace and populate disk cache
write(
Expand Down Expand Up @@ -126,13 +152,72 @@ private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOption
}

@Test
public void blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
blobsReferencedInAAreMissingCasIgnoresAc();
public void blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
}

@Test
public void bwob_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
doBlobsReferencedInAcAreMissingFromCasIgnoresAc("--remote_download_minimal");
}

@Test
public void bwob_blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
blobsReferencedInAAreMissingCasIgnoresAc("--remote_download_minimal");
public void bwobAndRemoteExec_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
startWorker();
enableRemoteExec("--remote_download_minimal");
doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
}

@Test
public void bwobAndRemoteCache_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
startWorker();
enableRemoteCache("--remote_download_minimal");
doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
}

private void doRemoteExecWithDiskCache(String... additionalOptions) throws Exception {
// Arrange: Prepare the workspace and populate disk cache
startWorker();
enableRemoteExec(additionalOptions);
write(
"BUILD",
"genrule(",
" name = 'foo',",
" srcs = ['foo.in'],",
" outs = ['foo.out'],",
" cmd = 'cat $(SRCS) > $@',",
")",
"genrule(",
" name = 'foobar',",
" srcs = [':foo.out', 'bar.in'],",
" outs = ['foobar.out'],",
" cmd = 'cat $(SRCS) > $@',",
")");
write("foo.in", "foo");
write("bar.in", "bar");
buildTarget("//:foobar");
cleanAndRestartServer();

// Act: Do a clean build
enableRemoteExec("--remote_download_minimal");
buildTarget("//:foobar");
}

@Test
public void remoteExecWithDiskCache_hitDiskCache() throws Exception {
doRemoteExecWithDiskCache();

// Assert: Should hit the disk cache
events.assertContainsInfo("2 disk cache hit");
}

@Test
public void bwob_remoteExecWithDiskCache_hitRemoteCache() throws Exception {
doRemoteExecWithDiskCache("--remote_download_minimal");

// Assert: Should hit the remote cache because blobs referenced by the AC are missing from disk
// cache due to BwoB.
events.assertContainsInfo("2 remote cache hit");
}

private void cleanAndRestartServer() throws Exception {
Expand Down

0 comments on commit 5f2866f

Please sign in to comment.