From ddf7d66a979ac9ca60e4eab6f69f0cbc8b7df59b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 26 Aug 2024 16:03:23 +0200 Subject: [PATCH 1/2] Add support for in-memory outputs to output reuse --- .../build/lib/actions/SpawnResult.java | 44 +++++++++----- .../lib/remote/RemoteExecutionService.java | 24 +++++++- .../lib/remote/RemoteSpawnCacheTest.java | 58 +++++++++++++++++-- 3 files changed, 104 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java index 4ffb4d233f6f9b..2315f8318a0798 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java @@ -38,7 +38,7 @@ @SuppressWarnings("GoodTime") // Use ints instead of Durations to improve build time (cl/505728570) public interface SpawnResult { - int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/ 128 + /*SIGALRM=*/ 14; + int POSIX_TIMEOUT_EXIT_CODE = /* SIGNAL_BASE= */ 128 + /* SIGALRM= */ 14; /** The status of the attempted Spawn execution. */ enum Status { @@ -260,16 +260,30 @@ default String getFailureMessage() { * *

This behavior may be triggered with {@link * ExecutionRequirements#REMOTE_EXECUTION_INLINE_OUTPUTS}. + * + *

Implementing classes should override {@link #getInMemoryOutputBytes(ActionInput)} instead. */ @Nullable default InputStream getInMemoryOutput(ActionInput output) { + ByteString bytes = getInMemoryOutputBytes(output); + if (bytes == null) { + return null; + } + return bytes.newInput(); + } + + /** + * Returns a {@link Spawn}'s output in-memory, if supported and available. + * + *

This behavior may be triggered with {@link + * ExecutionRequirements#REMOTE_EXECUTION_INLINE_OUTPUTS}. + */ + @Nullable + default ByteString getInMemoryOutputBytes(ActionInput output) { return null; } - String getDetailMessage( - String message, - boolean catastrophe, - boolean forciblyRunRemotely); + String getDetailMessage(String message, boolean catastrophe, boolean forciblyRunRemotely); /** Returns a file path to the action metadata log. */ @Nullable @@ -434,11 +448,8 @@ public String getFailureMessage() { @Override public String getDetailMessage( - String message, - boolean catastrophe, - boolean forciblyRunRemotely) { - TerminationStatus status = new TerminationStatus( - exitCode(), status() == Status.TIMEOUT); + String message, boolean catastrophe, boolean forciblyRunRemotely) { + TerminationStatus status = new TerminationStatus(exitCode(), status() == Status.TIMEOUT); String reason = "(" + status.toShortString() + ")"; // e.g. "(Exit 1)" String explanation = Strings.isNullOrEmpty(message) ? "" : ": " + message; @@ -457,17 +468,18 @@ public String getDetailMessage( explanation += " (Remote action was terminated due to Out of Memory.)"; } if (status() != Status.TIMEOUT && forciblyRunRemotely) { - explanation += " Action tagged as local was forcibly run remotely and failed - it's " - + "possible that the action simply doesn't work remotely"; + explanation += + " Action tagged as local was forcibly run remotely and failed - it's " + + "possible that the action simply doesn't work remotely"; } return reason + explanation; } @Nullable @Override - public InputStream getInMemoryOutput(ActionInput output) { + public ByteString getInMemoryOutputBytes(ActionInput output) { if (inMemoryOutputFile != null && inMemoryOutputFile.equals(output)) { - return inMemoryContents.newInput(); + return inMemoryContents; } return null; } @@ -603,8 +615,8 @@ public String getFailureMessage() { @Override @Nullable - public InputStream getInMemoryOutput(ActionInput output) { - return delegate.getInMemoryOutput(output); + public ByteString getInMemoryOutputBytes(ActionInput output) { + return delegate.getInMemoryOutputBytes(output); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index ebc4a72010bc0c..9f6244c3148966 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1458,12 +1458,20 @@ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution pr previousExecution.action.getSpawn().getOutputFiles().stream() .collect(toImmutableMap(output -> execRoot.getRelative(output.getExecPath()), o -> o)); Map realToTmpPath = new HashMap<>(); + ByteString inMemoryOutputContent = null; + String inMemoryOutputPath = null; try { for (String output : action.getCommand().getOutputPathsList()) { String reencodedOutput = reencodeExternalToInternal(output); Path sourcePath = previousExecution.action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput); ActionInput outputArtifact = previousOutputs.get(sourcePath); + Path targetPath = action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput); + inMemoryOutputContent = previousSpawnResult.getInMemoryOutputBytes(outputArtifact); + if (inMemoryOutputContent != null) { + inMemoryOutputPath = targetPath.relativeTo(execRoot).getPathString(); + continue; + } Path tmpPath = tempPathGenerator.generateTempPath(); tmpPath.getParentDirectory().createDirectoryAndParents(); try { @@ -1476,7 +1484,6 @@ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution pr FileSystemUtils.copyFile(sourcePath, tmpPath); } - Path targetPath = action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput); realToTmpPath.put(targetPath, tmpPath); } catch (FileNotFoundException e) { // The spawn this action was deduplicated against failed to create an output file. If the @@ -1517,6 +1524,21 @@ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution pr throw e; } + if (inMemoryOutputPath != null) { + String finalInMemoryOutputPath = inMemoryOutputPath; + ByteString finalInMemoryOutputContent = inMemoryOutputContent; + return new SpawnResult.DelegateSpawnResult(previousSpawnResult) { + @Override + @Nullable + public ByteString getInMemoryOutputBytes(ActionInput output) { + if (output.getExecPathString().equals(finalInMemoryOutputPath)) { + return finalInMemoryOutputContent; + } + return null; + } + }; + } + return previousSpawnResult; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index 2d14126fdcf678..2d438809b2dbef 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -93,6 +93,7 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.common.options.Options; +import com.google.protobuf.ByteString; import java.io.IOException; import java.time.Duration; import java.util.Set; @@ -833,9 +834,9 @@ public boolean mayModifySpawnOutputsAfterExecution() { try { secondCacheHandleRef.set(cache.lookup(secondSpawn, secondPolicy)); } catch (InterruptedException - | IOException - | ExecException - | ForbiddenActionInputException e) { + | IOException + | ExecException + | ForbiddenActionInputException e) { throw new IllegalStateException(e); } }); @@ -876,12 +877,59 @@ public boolean mayModifySpawnOutputsAfterExecution() { assertThat(secondCacheHandle.hasResult()).isTrue(); assertThat(secondCacheHandle.getResult().getRunnerName()).isEqualTo("deduplicated"); assertThat( - FileSystemUtils.readContent( - fs.getPath("/exec/root/bazel-bin/k8-opt/bin/output"), UTF_8)) + FileSystemUtils.readContent( + fs.getPath("/exec/root/bazel-bin/k8-opt/bin/output"), UTF_8)) .isEqualTo("hello"); assertThat(secondCacheHandle.willStore()).isFalse(); } + @Test + public void pathMappedActionWithInMemoryOutputIsDeduplicated() throws Exception { + // arrange + RemoteSpawnCache cache = createRemoteSpawnCache(); + + SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild"); + FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot); + firstFakeFileCache.createScratchInput(firstSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext firstPolicy = + createSpawnExecutionContext(firstSpawn, execRoot, firstFakeFileCache, outErr); + + SimpleSpawn secondSpawn = simplePathMappedSpawn("k8-opt"); + FakeActionInputFileCache secondFakeFileCache = new FakeActionInputFileCache(execRoot); + secondFakeFileCache.createScratchInput(secondSpawn.getInputFiles().getSingleton(), "xyz"); + SpawnExecutionContext secondPolicy = + createSpawnExecutionContext(secondSpawn, execRoot, secondFakeFileCache, outErr); + + RemoteExecutionService remoteExecutionService = cache.getRemoteExecutionService(); + Mockito.doCallRealMethod().when(remoteExecutionService).waitForAndReuseOutputs(any(), any()); + // Simulate a very slow upload to the remote cache to ensure that the second spawn is + // deduplicated rather than a cache hit. This is a slight hack, but also avoid introducing + // concurrency to this test. + Mockito.doNothing().when(remoteExecutionService).uploadOutputs(any(), any(), any()); + + // act + try (CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy)) { + firstCacheHandle.store( + new SpawnResult.Builder() + .setExitCode(0) + .setStatus(Status.SUCCESS) + .setRunnerName("test") + .setInMemoryOutput( + firstSpawn.getOutputFiles().getFirst(), ByteString.copyFrom("in-memory", UTF_8)) + .build()); + } + CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy); + + // assert + ActionInput inMemoryOutput = secondSpawn.getOutputFiles().getFirst(); + assertThat(secondCacheHandle.hasResult()).isTrue(); + assertThat(secondCacheHandle.getResult().getRunnerName()).isEqualTo("deduplicated"); + assertThat(secondCacheHandle.getResult().getInMemoryOutputBytes(inMemoryOutput).toStringUtf8()) + .isEqualTo("in-memory"); + assertThat(execRoot.getRelative(inMemoryOutput.getExecPath()).exists()).isFalse(); + assertThat(secondCacheHandle.willStore()).isFalse(); + } + @Test public void deduplicatedActionWithNonZeroExitCodeIsACacheMiss() throws Exception { // arrange From 3dfd5e5d1a900e22f1ac89856e9d26c3e2e28400 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 28 Aug 2024 16:21:26 +0200 Subject: [PATCH 2/2] Address comments --- .../build/lib/actions/SpawnResult.java | 26 +++---------------- .../lib/analysis/actions/StarlarkAction.java | 7 ++--- .../includescanning/SpawnIncludeScanner.java | 7 ++++- .../lib/remote/RemoteExecutionService.java | 4 +-- .../build/lib/rules/cpp/CppCompileAction.java | 14 ++++------ .../lib/rules/java/JavaCompileAction.java | 5 ++-- .../build/lib/actions/SpawnResultTest.java | 4 +-- .../lib/remote/RemoteSpawnCacheTest.java | 4 +-- 8 files changed, 28 insertions(+), 43 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java index 2315f8318a0798..cde538c9de102e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java @@ -29,7 +29,6 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.protobuf.ByteString; import java.io.IOException; -import java.io.InputStream; import java.time.Instant; import java.util.Locale; import javax.annotation.Nullable; @@ -255,23 +254,6 @@ default String getFailureMessage() { return ""; } - /** - * Returns a {@link Spawn}'s output in-memory, if supported and available. - * - *

This behavior may be triggered with {@link - * ExecutionRequirements#REMOTE_EXECUTION_INLINE_OUTPUTS}. - * - *

Implementing classes should override {@link #getInMemoryOutputBytes(ActionInput)} instead. - */ - @Nullable - default InputStream getInMemoryOutput(ActionInput output) { - ByteString bytes = getInMemoryOutputBytes(output); - if (bytes == null) { - return null; - } - return bytes.newInput(); - } - /** * Returns a {@link Spawn}'s output in-memory, if supported and available. * @@ -279,7 +261,7 @@ default InputStream getInMemoryOutput(ActionInput output) { * ExecutionRequirements#REMOTE_EXECUTION_INLINE_OUTPUTS}. */ @Nullable - default ByteString getInMemoryOutputBytes(ActionInput output) { + default ByteString getInMemoryOutput(ActionInput output) { return null; } @@ -477,7 +459,7 @@ public String getDetailMessage( @Nullable @Override - public ByteString getInMemoryOutputBytes(ActionInput output) { + public ByteString getInMemoryOutput(ActionInput output) { if (inMemoryOutputFile != null && inMemoryOutputFile.equals(output)) { return inMemoryContents; } @@ -615,8 +597,8 @@ public String getFailureMessage() { @Override @Nullable - public ByteString getInMemoryOutputBytes(ActionInput output) { - return delegate.getInMemoryOutputBytes(output); + public ByteString getInMemoryOutput(ActionInput output) { + return delegate.getInMemoryOutput(output); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java index 7fdc3ad494c74a..7288670b9ff7c7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.StarlarkAction.Code; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.protobuf.ByteString; import java.io.BufferedReader; import java.io.FileNotFoundException; import java.io.IOException; @@ -312,9 +313,9 @@ private InputStream getUnusedInputListInputStream( // Note: SpawnActionContext guarantees that the first list entry exists and corresponds to the // executed spawn. Artifact unusedInputsListArtifact = unusedInputsList.get(); - InputStream inputStream = spawnResults.get(0).getInMemoryOutput(unusedInputsListArtifact); - if (inputStream != null) { - return inputStream; + ByteString content = spawnResults.get(0).getInMemoryOutput(unusedInputsListArtifact); + if (content != null) { + return content.newInput(); } // Fallback to reading from disk. try { diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java index f154653bf3012e..f58073d018204c 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.SyscallCache; +import com.google.protobuf.ByteString; import java.io.IOException; import java.io.InputStream; import java.util.Collection; @@ -402,7 +403,11 @@ private static InputStream spawnGrep( } SpawnResult result = results.getFirst(); - return result.getInMemoryOutput(output); + ByteString includesContent = result.getInMemoryOutput(output); + if (includesContent != null) { + return includesContent.newInput(); + } + return null; } private static void dump(ActionExecutionContext fromContext, ActionExecutionContext toContext) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 9f6244c3148966..bc9a7d0a5588f4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1467,7 +1467,7 @@ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution pr previousExecution.action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput); ActionInput outputArtifact = previousOutputs.get(sourcePath); Path targetPath = action.getRemotePathResolver().outputPathToLocalPath(reencodedOutput); - inMemoryOutputContent = previousSpawnResult.getInMemoryOutputBytes(outputArtifact); + inMemoryOutputContent = previousSpawnResult.getInMemoryOutput(outputArtifact); if (inMemoryOutputContent != null) { inMemoryOutputPath = targetPath.relativeTo(execRoot).getPathString(); continue; @@ -1530,7 +1530,7 @@ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution pr return new SpawnResult.DelegateSpawnResult(previousSpawnResult) { @Override @Nullable - public ByteString getInMemoryOutputBytes(ActionInput output) { + public ByteString getInMemoryOutput(ActionInput output) { if (output.getExecPathString().equals(finalInMemoryOutputPath)) { return finalInMemoryOutputContent; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 859daa1f8f2f45..533b2d96c225b7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -96,6 +96,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; +import com.google.protobuf.ByteString; import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; @@ -1508,16 +1509,11 @@ e, createFailureDetail("OutErr copy failure", Code.COPY_OUT_ERR_FAILURE)), } @Nullable - private byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException { + private byte[] getDotDContents(SpawnResult spawnResult) { if (getDotdFile() != null) { - InputStream in = spawnResult.getInMemoryOutput(getDotdFile()); - if (in != null) { - try { - return ByteStreams.toByteArray(in); - } catch (IOException e) { - throw new EnvironmentalExecException( - e, createFailureDetail("Reading in-memory .d file failed", Code.D_FILE_READ_FAILURE)); - } + ByteString content = spawnResult.getInMemoryOutput(getDotdFile()); + if (content != null) { + return content.toByteArray(); } } return null; diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 5db91c5f75888b..6081e0b393447a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -82,6 +82,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.proto.Deps; +import com.google.protobuf.ByteString; import com.google.protobuf.ExtensionRegistry; import java.io.IOException; import java.io.InputStream; @@ -842,11 +843,11 @@ private static Deps.Dependencies readExecutorJdeps( Artifact outputDepsProto, ActionExecutionContext actionExecutionContext) throws IOException { - InputStream inMemoryOutput = spawnResult.getInMemoryOutput(outputDepsProto); + ByteString inMemoryOutput = spawnResult.getInMemoryOutput(outputDepsProto); try (InputStream inputStream = inMemoryOutput == null ? actionExecutionContext.getInputPath(outputDepsProto).getInputStream() - : inMemoryOutput) { + : inMemoryOutput.newInput()) { return Deps.Dependencies.parseFrom(inputStream, ExtensionRegistry.getEmptyRegistry()); } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java index df9e2b868831e3..04568054a4a23c 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java @@ -66,7 +66,7 @@ public void getTimeoutMessageNoTime() { } @Test - public void inMemoryContents() throws Exception { + public void inMemoryContents() { ActionInput output = ActionInputHelper.fromPath("/foo/bar"); ByteString contents = ByteString.copyFromUtf8("hello world"); @@ -78,7 +78,7 @@ public void inMemoryContents() throws Exception { .setInMemoryOutput(output, contents) .build(); - assertThat(ByteString.readFrom(r.getInMemoryOutput(output))).isEqualTo(contents); + assertThat(r.getInMemoryOutput(output)).isEqualTo(contents); assertThat(r.getInMemoryOutput(null)).isEqualTo(null); assertThat(r.getInMemoryOutput(ActionInputHelper.fromPath("/does/not/exist"))).isEqualTo(null); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index 2d438809b2dbef..d194747060f439 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -915,7 +915,7 @@ public void pathMappedActionWithInMemoryOutputIsDeduplicated() throws Exception .setStatus(Status.SUCCESS) .setRunnerName("test") .setInMemoryOutput( - firstSpawn.getOutputFiles().getFirst(), ByteString.copyFrom("in-memory", UTF_8)) + firstSpawn.getOutputFiles().getFirst(), ByteString.copyFromUtf8("in-memory")) .build()); } CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy); @@ -924,7 +924,7 @@ public void pathMappedActionWithInMemoryOutputIsDeduplicated() throws Exception ActionInput inMemoryOutput = secondSpawn.getOutputFiles().getFirst(); assertThat(secondCacheHandle.hasResult()).isTrue(); assertThat(secondCacheHandle.getResult().getRunnerName()).isEqualTo("deduplicated"); - assertThat(secondCacheHandle.getResult().getInMemoryOutputBytes(inMemoryOutput).toStringUtf8()) + assertThat(secondCacheHandle.getResult().getInMemoryOutput(inMemoryOutput).toStringUtf8()) .isEqualTo("in-memory"); assertThat(execRoot.getRelative(inMemoryOutput.getExecPath()).exists()).isFalse(); assertThat(secondCacheHandle.willStore()).isFalse();