From 5f2866f8434ce9a17cf82c001efb7b236f189115 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 18 Jan 2023 02:41:02 -0800 Subject: [PATCH] Do the AC integrity check for disk part of the combined cache. 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 --- .../remote/disk/DiskAndRemoteCacheClient.java | 60 ++++++++---- .../lib/remote/disk/DiskCacheClient.java | 11 +-- .../google/devtools/build/lib/remote/BUILD | 5 + .../lib/remote/DiskCacheIntegrationTest.java | 95 ++++++++++++++++++- 4 files changed, 141 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java index 1fc6200cd9e9d7..37ccd2cd4b456a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java @@ -183,31 +183,53 @@ public ListenableFuture downloadBlob( } } + private ListenableFuture 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 downloadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { - if (context.getReadCachePolicy().allowDiskCache() - && diskCache.containsActionResult(actionKey)) { - return diskCache.downloadActionResult(context, actionKey, inlineOutErr); + ListenableFuture 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; } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index d0d6bd406d7e9c..55d6b43581f8dd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -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(); @@ -161,7 +156,11 @@ public ListenableFuture downloadActionResult( } if (checkActionResult) { - checkActionResult(actionResult); + try { + checkActionResult(actionResult); + } catch (CacheNotFoundException e) { + return Futures.immediateFuture(null); + } } return Futures.immediateFuture(CachedActionResult.disk(actionResult)); diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 7cef85b3320396..f438d4dce01bc2 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -171,6 +171,9 @@ 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", @@ -178,8 +181,10 @@ java_test( "//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", ], ) diff --git a/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java index ee64e175c495dd..5c6bf046b48529 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java @@ -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; @@ -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()); @@ -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 @@ -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( @@ -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 {