Skip to content

Commit

Permalink
Do the AC integrity check for disk part of the combined cache. (#17309)
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 #17080 for reasoning.

With lease service, the decrement will be fixed.

Fixes #17080.

Closes #17201.

PiperOrigin-RevId: 502818641
Change-Id: I1f73dd38d7e52e2f39b367e6114aab714de22d3c

Co-authored-by: Chi Wang <chiwang@google.com>
  • Loading branch information
@ShreeM01 @coeuvre
ShreeM01 and coeuvre authored Jan 25, 2023
1 parent 49e8ed1 commit 97f6481
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 97f6481

Please sign in to comment.