From 02838a1b2aa2f6d03980536ab2ac6840c3c98e84 Mon Sep 17 00:00:00 2001 From: larsrc Date: Tue, 10 Nov 2020 03:46:53 -0800 Subject: [PATCH] Avoid the spawn cache if executing dynamically. Fixes bazelbuild#12364 (caused by https://github.com/bazelbuild/bazel/commit/25e58ffc529b9cf4e1d0dccbbad27f40f092f1ec) based on Ulf's approach, but avoiding secretly disabling some potential caches that should not be disabled. RELNOTES: n/a PiperOrigin-RevId: 341588598 --- .../build/lib/exec/AbstractSpawnStrategy.java | 12 ++- .../devtools/build/lib/exec/SpawnCache.java | 11 +++ .../build/lib/remote/RemoteSpawnCache.java | 5 ++ .../java/com/google/devtools/build/lib/BUILD | 1 + .../lib/exec/AbstractSpawnStrategyTest.java | 80 +++++++++++++++++++ 5 files changed, 106 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index f55a1b685f95e1..84fffd9d59bfa4 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -114,14 +114,20 @@ public ImmutableList exec( SpawnExecutionContext context = new SpawnExecutionContextImpl(spawn, actionExecutionContext, stopConcurrentSpawns, timeout); - SpawnCache cache = actionExecutionContext.getContext(SpawnCache.class); + // Avoid caching for runners which handle caching internally e.g. RemoteSpawnRunner. + SpawnCache cache = + spawnRunner.handlesCaching() + ? SpawnCache.NO_CACHE + : actionExecutionContext.getContext(SpawnCache.class); + // In production, the getContext method guarantees that we never get null back. However, our // integration tests don't set it up correctly, so cache may be null in testing. if (cache == null) { cache = SpawnCache.NO_CACHE; } - // Avoid caching for runners which handle caching internally e.g. RemoteSpawnRunner - if (spawnRunner.handlesCaching()) { + + // Avoid using the remote cache of a dynamic execution setup for the local runner. + if (context.speculating() && !cache.usefulInDynamicExecution()) { cache = SpawnCache.NO_CACHE; } SpawnResult spawnResult; diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java index af64f4b9df99d9..b47c8674a8a7ae 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java @@ -165,4 +165,15 @@ interface CacheHandle extends Closeable { */ CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) throws ExecException, IOException, InterruptedException; + + /** + * Returns whether this cache implementation makes sense to use together with dynamic execution. + * + *

A cache that's part of the remote system used for dynamic execution should not also be used + * for the local speculative execution. However, a local cache or a separate remote cache-only + * system would be. + */ + default boolean usefulInDynamicExecution() { + return true; + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index e72c04efb041d8..51a1d659035784 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -342,4 +342,9 @@ private static boolean useRemoteCache(RemoteOptions options) { private static boolean useDiskCache(RemoteOptions options) { return options.diskCache != null && !options.diskCache.isEmpty(); } + + @Override + public boolean usefulInDynamicExecution() { + return false; + } } diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index efc78072534639..41dc5a26a33329 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -82,6 +82,7 @@ java_library( "//src/test/java/com/google/devtools/build/lib/testutil", "//src/test/java/com/google/devtools/build/lib/testutil:TestSuite", "//third_party:junit4", + "//third_party:mockito", ], ) diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java index 69011369e524c0..389b5ec2460203 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java @@ -20,6 +20,8 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -183,6 +185,84 @@ public void testCacheMiss() throws Exception { verify(entry).store(eq(spawnResult)); } + @Test + public void testExec_whenLocalCaches_usesNoCache() throws Exception { + when(spawnRunner.handlesCaching()).thenReturn(true); + + SpawnCache cache = mock(SpawnCache.class); + + when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(cache); + when(actionExecutionContext.getExecRoot()).thenReturn(execRoot); + SpawnResult spawnResult = + new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build(); + when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class))) + .thenReturn(FutureSpawn.immediate(spawnResult)); + + List spawnResults = + new TestedSpawnStrategy(execRoot, spawnRunner).exec(SIMPLE_SPAWN, actionExecutionContext); + + assertThat(spawnResults).containsExactly(spawnResult); + + // Must only be called exactly once. + verify(spawnRunner).execAsync(any(Spawn.class), any(SpawnExecutionContext.class)); + verifyZeroInteractions(cache); + } + + @Test + public void testExec_usefulCacheInDynamicExecution() throws Exception { + when(spawnRunner.handlesCaching()).thenReturn(false); + + SpawnCache cache = mock(SpawnCache.class); + when(cache.usefulInDynamicExecution()).thenReturn(true); + CacheHandle entry = mock(CacheHandle.class); + when(cache.lookup(any(Spawn.class), any(SpawnExecutionContext.class))).thenReturn(entry); + when(entry.hasResult()).thenReturn(false); + when(entry.willStore()).thenReturn(true); + + when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(cache); + when(actionExecutionContext.getExecRoot()).thenReturn(execRoot); + SpawnResult spawnResult = + new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build(); + when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class))) + .thenReturn(FutureSpawn.immediate(spawnResult)); + + List spawnResults = + new TestedSpawnStrategy(execRoot, spawnRunner) + .exec(SIMPLE_SPAWN, actionExecutionContext, () -> {}); + + assertThat(spawnResults).containsExactly(spawnResult); + + // Must only be called exactly once. + verify(spawnRunner).execAsync(any(Spawn.class), any(SpawnExecutionContext.class)); + verify(entry).store(eq(spawnResult)); + } + + @Test + public void testExec_nonUsefulCacheInDynamicExecution() throws Exception { + when(spawnRunner.handlesCaching()).thenReturn(false); + + SpawnCache cache = mock(SpawnCache.class); + when(cache.usefulInDynamicExecution()).thenReturn(false); + + when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(cache); + when(actionExecutionContext.getExecRoot()).thenReturn(execRoot); + SpawnResult spawnResult = + new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build(); + when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class))) + .thenReturn(FutureSpawn.immediate(spawnResult)); + + List spawnResults = + new TestedSpawnStrategy(execRoot, spawnRunner) + .exec(SIMPLE_SPAWN, actionExecutionContext, () -> {}); + + assertThat(spawnResults).containsExactly(spawnResult); + + // Must only be called exactly once. + verify(spawnRunner).execAsync(any(Spawn.class), any(SpawnExecutionContext.class)); + verify(cache).usefulInDynamicExecution(); + verifyNoMoreInteractions(cache); + } + @Test public void testCacheMissWithNonZeroExit() throws Exception { SpawnCache cache = mock(SpawnCache.class);