diff options
Diffstat (limited to 'src')
10 files changed, 39 insertions, 59 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 953ae1647b..ed15ed5d17 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -229,7 +229,7 @@ public class ExecutionTool { this.actionContextProviders = builder.getActionContextProviders(); for (ActionContextProvider provider : actionContextProviders) { - provider.init(fileCache, prefetcher); + provider.init(fileCache); } StrategyConverter strategyConverter = new StrategyConverter(actionContextProviders); 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 ffef030501..5aacd8ab67 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 @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.exec; +import com.google.common.base.Predicates; +import com.google.common.collect.Iterables; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionInput; @@ -72,6 +74,9 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte final Duration timeout = Spawns.getTimeout(spawn); SpawnExecutionPolicy policy = new SpawnExecutionPolicy() { private final int id = execCount.incrementAndGet(); + // Memoize the input mapping so that prefetchInputs can reuse it instead of recomputing it. + // TODO(ulfjack): Guard against client modification of this map. + private SortedMap<PathFragment, ActionInput> lazyInputMapping; @Override public int getId() { @@ -79,8 +84,14 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte } @Override - public void prefetchInputs(Iterable<ActionInput> inputs) throws IOException { - actionExecutionContext.getActionInputPrefetcher().prefetchFiles(inputs); + public void prefetchInputs() throws IOException { + if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) { + // TODO(philwo): Benchmark whether using an ExecutionService to do multiple operations in + // parallel speeds up prefetching of inputs. + // TODO(philwo): Do we have to expand middleman artifacts here? + actionExecutionContext.getActionInputPrefetcher().prefetchFiles( + Iterables.filter(getInputMapping().values(), Predicates.notNull())); + } } @Override @@ -115,11 +126,14 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte @Override public SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException { - return spawnInputExpander.getInputMapping( - spawn, - actionExecutionContext.getArtifactExpander(), - actionExecutionContext.getActionInputFileCache(), - actionExecutionContext.getContext(FilesetActionContext.class)); + if (lazyInputMapping == null) { + lazyInputMapping = spawnInputExpander.getInputMapping( + spawn, + actionExecutionContext.getArtifactExpander(), + actionExecutionContext.getActionInputFileCache(), + actionExecutionContext.getContext(FilesetActionContext.class)); + } + return lazyInputMapping; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/exec/ActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/exec/ActionContextProvider.java index 27e147b701..f2f5e0f6fb 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ActionContextProvider.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.exec; import com.google.devtools.build.lib.actions.ActionContext; import com.google.devtools.build.lib.actions.ActionGraph; import com.google.devtools.build.lib.actions.ActionInputFileCache; -import com.google.devtools.build.lib.actions.ActionInputPrefetcher; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ExecutorInitException; @@ -35,15 +34,13 @@ public abstract class ActionContextProvider { public abstract Iterable<? extends ActionContext> getActionContexts(); /** - * Two-phase initialization. The input file cache and the input prefetcher usually come from a - * different module than the {@link ActionContextProvider} instances that require them, so this - * method is called after {@link com.google.devtools.build.lib.runtime.BlazeModule#executorInit}. + * Two-phase initialization. The input file cache usually comes from a different module than the + * {@link ActionContextProvider} instances that require it, so this method is called after + * {@link com.google.devtools.build.lib.runtime.BlazeModule#executorInit}. * * @param actionInputFileCache the input file cache - * @param actionInputPrefetcher the input file prefetcher */ - public void init( - ActionInputFileCache actionInputFileCache, ActionInputPrefetcher actionInputPrefetcher) { + public void init(ActionInputFileCache actionInputFileCache) { } /** diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java index b98ca2ee59..e7f67042f0 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java @@ -147,7 +147,7 @@ public interface SpawnRunner { * again. I suppose we could require implementations to memoize getInputMapping (but not compute * it eagerly), and that may change in the future. */ - void prefetchInputs(Iterable<ActionInput> inputs) throws IOException; + void prefetchInputs() throws IOException; /** * The input file metadata cache for this specific spawn, which can be used to efficiently diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java index 548b3b4091..57d757691d 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java @@ -18,8 +18,6 @@ import static java.util.logging.Level.INFO; import static java.util.logging.Level.SEVERE; import com.google.common.base.Joiner; -import com.google.common.base.Predicates; -import com.google.common.collect.Iterables; import com.google.common.io.ByteStreams; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ResourceManager; @@ -230,8 +228,7 @@ public final class LocalSpawnRunner implements SpawnRunner { if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) { stepLog(INFO, "prefetching inputs for local execution"); setState(State.PREFETCHING_LOCAL_INPUTS); - policy.prefetchInputs( - Iterables.filter(policy.getInputMapping().values(), Predicates.notNull())); + policy.prefetchInputs(); } stepLog(INFO, "running locally"); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java index 7fb7274e30..6ac4a6e319 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java @@ -90,10 +90,11 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { try { sandbox.createFileSystem(); OutErr outErr = policy.getFileOutErr(); + policy.prefetchInputs(); + SpawnResult result = run(sandbox, outErr, timeout); - + policy.lockOutputFiles(); - try { // We copy the outputs even when the command failed. sandbox.copyOutputs(execRoot); diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index e822904a17..dd5c231036 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java @@ -22,12 +22,10 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.io.ByteStreams; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputFileCache; -import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.ResourceManager; @@ -145,7 +143,7 @@ public class LocalSpawnRunnerTest { private final TreeMap<PathFragment, ActionInput> inputMapping = new TreeMap<>(); private long timeoutMillis; - private final List<Iterable<ActionInput>> prefetched = new ArrayList<>(); + private boolean prefetchCalled; private boolean lockOutputFilesCalled; @Override @@ -154,8 +152,8 @@ public class LocalSpawnRunnerTest { } @Override - public void prefetchInputs(Iterable<ActionInput> inputs) throws IOException { - prefetched.add(Preconditions.checkNotNull(inputs)); + public void prefetchInputs() throws IOException { + prefetchCalled = true; } @Override @@ -435,7 +433,7 @@ public class LocalSpawnRunnerTest { policy.timeoutMillis = 123 * 1000L; outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); runner.exec(SIMPLE_SPAWN, policy); - assertThat(policy.prefetched).isNotEmpty(); + assertThat(policy.prefetchCalled).isTrue(); } @Test @@ -455,34 +453,7 @@ public class LocalSpawnRunnerTest { Spawn spawn = new SpawnBuilder("/bin/echo", "Hi!") .withExecutionInfo(ExecutionRequirements.DISABLE_LOCAL_PREFETCH, "").build(); runner.exec(spawn, policy); - assertThat(policy.prefetched).isEmpty(); - } - - /** - * Regression test: the SpawnInputExpander can return null values for empty files, but the - * ActionInputPrefetcher expects no null values. - */ - @Test - public void checkPrefetchCalledNonNull() throws Exception { - Subprocess.Factory factory = mock(Subprocess.Factory.class); - when(factory.create(any())).thenReturn(new FinishedSubprocess(0)); - SubprocessBuilder.setSubprocessFactory(factory); - - LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class); - LocalSpawnRunner runner = new LocalSpawnRunner( - fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX, - "product-name", LocalEnvProvider.UNMODIFIED); - - policy.inputMapping.put(PathFragment.create("relative/path"), null); - policy.inputMapping.put( - PathFragment.create("another/relative/path"), ActionInputHelper.fromPath("/absolute/path")); - policy.timeoutMillis = 123 * 1000L; - outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - runner.exec(SIMPLE_SPAWN, policy); - assertThat(policy.prefetched).hasSize(1); - Iterable<ActionInput> prefetched = policy.prefetched.get(0); - assertThat(prefetched).doesNotContain(null); - assertThat(prefetched).containsExactly(ActionInputHelper.fromPath("/absolute/path")); + assertThat(policy.prefetchCalled).isFalse(); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java index 6a1fdb4bb4..746447b1a6 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java @@ -87,7 +87,7 @@ public class CachedLocalSpawnRunnerTest { } @Override - public void prefetchInputs(Iterable<ActionInput> inputs) { + public void prefetchInputs() { // CachedLocalSpawnRunner should never prefetch itself, though the nested SpawnRunner may. throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java index 6dc3f15e97..e04630a8c0 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java @@ -119,7 +119,7 @@ public class GrpcRemoteExecutionClientTest { } @Override - public void prefetchInputs(Iterable<ActionInput> inputs) { + public void prefetchInputs() { throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 4c7fa6364c..2475b2cb1e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -232,7 +232,7 @@ public class RemoteSpawnRunnerTest { } @Override - public void prefetchInputs(Iterable<ActionInput> inputs) throws IOException { + public void prefetchInputs() throws IOException { throw new UnsupportedOperationException(); } |