Skip to content

Commit

Permalink
[7.4.0] Output reuse (#23674)
Browse files Browse the repository at this point in the history
Cherry-picks the following changes to implement output reuse:
* Deduplicate locally executed path mapped spawns (#22556)
* Fix local execution deduplication to work with optional outputs
(#23296)
* Force synchronous upload and reuse of possibly modified spawn outputs
(#23382)
* Add support for in-memory outputs to output reuse (#23422)

Fixes #23377
Fixes #23444
Fixes #23457
  • Loading branch information
fmeum authored Sep 19, 2024
1 parent 56fedfd commit d03dca6
Show file tree
Hide file tree
Showing 20 changed files with 1,310 additions and 228 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,14 @@ default String getProgressMessage(RepositoryMapping mainRepositoryMapping) {
default boolean mayInsensitivelyPropagateInputs() {
return false;
}

/**
* Returns true if the action may modify spawn outputs after the spawn has executed.
*
* <p>If this returns true, any kind of spawn output caching or reuse needs to happen
* synchronously directly after the spawn execution.
*/
default boolean mayModifySpawnOutputsAfterExecution() {
return false;
}
}
169 changes: 153 additions & 16 deletions src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,7 +37,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 {
Expand Down Expand Up @@ -262,14 +261,11 @@ default String getFailureMessage() {
* ExecutionRequirements#REMOTE_EXECUTION_INLINE_OUTPUTS}.
*/
@Nullable
default InputStream getInMemoryOutput(ActionInput output) {
default ByteString getInMemoryOutput(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
Expand Down Expand Up @@ -434,11 +430,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;

Expand All @@ -457,17 +450,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 getInMemoryOutput(ActionInput output) {
if (inMemoryOutputFile != null && inMemoryOutputFile.equals(output)) {
return inMemoryContents.newInput();
return inMemoryContents;
}
return null;
}
Expand All @@ -488,6 +482,149 @@ public Digest getDigest() {
}
}

/**
* A helper class for wrapping an existing {@link SpawnResult} and modifying a subset of its
* methods.
*/
class DelegateSpawnResult implements SpawnResult {
private final SpawnResult delegate;

public DelegateSpawnResult(SpawnResult delegate) {
this.delegate = delegate;
}

@Override
public boolean setupSuccess() {
return delegate.setupSuccess();
}

@Override
public boolean isCatastrophe() {
return delegate.isCatastrophe();
}

@Override
public Status status() {
return delegate.status();
}

@Override
public int exitCode() {
return delegate.exitCode();
}

@Override
@Nullable
public FailureDetail failureDetail() {
return delegate.failureDetail();
}

@Override
@Nullable
public String getExecutorHostName() {
return delegate.getExecutorHostName();
}

@Override
public String getRunnerName() {
return delegate.getRunnerName();
}

@Override
public String getRunnerSubtype() {
return delegate.getRunnerSubtype();
}

@Override
@Nullable
public Instant getStartTime() {
return delegate.getStartTime();
}

@Override
public int getWallTimeInMs() {
return delegate.getWallTimeInMs();
}

@Override
public int getUserTimeInMs() {
return delegate.getUserTimeInMs();
}

@Override
public int getSystemTimeInMs() {
return delegate.getSystemTimeInMs();
}

@Override
@Nullable
public Long getNumBlockOutputOperations() {
return delegate.getNumBlockOutputOperations();
}

@Override
@Nullable
public Long getNumBlockInputOperations() {
return delegate.getNumBlockInputOperations();
}

@Override
@Nullable
public Long getNumInvoluntaryContextSwitches() {
return delegate.getNumInvoluntaryContextSwitches();
}

@Override
@Nullable
public Long getMemoryInKb() {
return delegate.getMemoryInKb();
}

@Override
public SpawnMetrics getMetrics() {
return delegate.getMetrics();
}

@Override
public boolean isCacheHit() {
return delegate.isCacheHit();
}

@Override
public String getFailureMessage() {
return delegate.getFailureMessage();
}

@Override
@Nullable
public ByteString getInMemoryOutput(ActionInput output) {
return delegate.getInMemoryOutput(output);
}

@Override
public String getDetailMessage(
String message, boolean catastrophe, boolean forciblyRunRemotely) {
return delegate.getDetailMessage(message, catastrophe, forciblyRunRemotely);
}

@Override
@Nullable
public MetadataLog getActionMetadataLog() {
return delegate.getActionMetadataLog();
}

@Override
public boolean wasRemote() {
return delegate.wasRemote();
}

@Override
@Nullable
public Digest getDigest() {
return delegate.getDigest();
}
}

/** Builder class for {@link SpawnResult}. */
final class Builder {
private int exitCode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,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;
Expand Down Expand Up @@ -315,9 +316,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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,16 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
this.isExecutedOnWindows = isExecutedOnWindows;
}

@Override
public boolean mayModifySpawnOutputsAfterExecution() {
// Test actions modify test spawn outputs after execution:
// - if there are multiple attempts (unavoidable);
// - in all cases due to appending any stray stderr output to the test log in
// StandaloneTestStrategy.
// TODO: Get rid of the second case and only return true if there are multiple attempts.
return true;
}

public boolean isExecutedOnWindows() {
return isExecutedOnWindows;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public ImmutableList<SpawnResult> exec(
? resultMessage
: CommandFailureUtils.describeCommandFailure(
executionOptions.verboseFailures, cwd, spawn);
throw new SpawnExecException(message, spawnResult, /*forciblyRunRemotely=*/ false);
throw new SpawnExecException(message, spawnResult, /* forciblyRunRemotely= */ false);
}
return ImmutableList.of(spawnResult);
}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/exec/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ java_library(
deps = [
":spawn_runner",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/profiler",
],
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.Spawns;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import java.io.Closeable;
import java.io.IOException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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;
Expand Down Expand Up @@ -351,6 +352,7 @@ public Collection<Inclusion> extractInclusions(
* Otherwise "null"
* @throws ExecException if scanning fails
*/
@Nullable
private static InputStream spawnGrep(
Artifact input,
PathFragment outputExecPath,
Expand Down Expand Up @@ -412,7 +414,11 @@ private static InputStream spawnGrep(
}

SpawnResult result = Iterables.getLast(results);
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) {
Expand Down
Loading

0 comments on commit d03dca6

Please sign in to comment.