From d4542bc78713c2c36c61924bbba4f2673cfa077b Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Tue, 10 Jan 2023 07:34:02 -0800 Subject: [PATCH] Make Bazel more responsive and use less memory when --jobs is high When using Bazel in combination with a larger remote execution cluster, it's not uncommon to call it with something like --jobs=512. We have observed that this is currently problematic for a couple of reasons: 1. It causes Bazel to launch 512 local threads, each being responsible for running one action remotely. All of these local threads may spend a lot of time in buildRemoteAction(), generating input roots in the form of Merkle trees. As the local system tends to have fewer than 512 CPUs, all of these threads will unnecessarily compete with each other. One practical downside of that is that interrupting Bazel using ^C takes a very long time, as it first wants to complete the computation of all 512 Merkle trees. Let's put a semaphore in place, limiting the number of concurrent Merkle tree computations to the number of CPU cores available. By making the semaphore interruptible, Bazel will immediately stop processing the `512 - nCPU` actions that were waiting for the semaphore. 2. Related to the above, Bazel will end up keeping 512 Merkle trees in memory throughout all stages of execution. This makes sense, as we may get cache misses, requiring us to upload the input root afterwards. Or the execution of a remote action may fail, requiring us to upload the input root. That said, generally speaking these cases are fairly uncommon. Most builds have relatively high cache hit rates and execution retries only happen rarely. It is therefore not worth keeping these Merkle trees in memory constantly. We only need it when computing the action digest for GetActionResult(), and while uploading it into the CAS. 3. AbstractSpawnStrategy.getInputMapping() has some smartness to memoize its results. This makes a lot of sense for local execution, where the input mapping is used in a couple of places. For remote caching/execution it is not evident that this is a good idea. Assuming you end up having a remote cache hit, you don't need it. Let's make the memoization optional, only using it in cases where we do local execution (which may also happen when you get a cache miss when doing remote caching without remote exection). Similar changes against Bazel 5.x have allowed me to successfully do builds of a large monorepo using --jobs=512 using the default heap size limits, whereas I would normally see occasional OOM behaviour when providing --host_jvm_args=-Xmx64g. Closes #17120. PiperOrigin-RevId: 500990181 Change-Id: I6d1ba03470b79424ce2e1c2e83abd8fa779dd268 --- .../build/lib/exec/AbstractSpawnStrategy.java | 43 +++-- .../devtools/build/lib/exec/SpawnRunner.java | 3 +- .../build/lib/remote/RemoteAction.java | 21 ++- .../lib/remote/RemoteExecutionService.java | 176 +++++++++++------- .../build/lib/remote/RemoteSpawnCache.java | 2 +- .../build/lib/remote/RemoteSpawnRunner.java | 4 +- .../lib/remote/common/RemotePathResolver.java | 14 +- .../lib/remote/options/RemoteOptions.java | 12 ++ .../sandbox/DarwinSandboxedSpawnRunner.java | 2 +- .../sandbox/DockerSandboxedSpawnRunner.java | 2 +- .../sandbox/LinuxSandboxedSpawnRunner.java | 9 +- .../ProcessWrapperSandboxedSpawnRunner.java | 2 +- .../sandbox/WindowsSandboxedSpawnRunner.java | 2 +- .../build/lib/worker/WorkerSpawnRunner.java | 4 +- .../lib/exec/local/LocalSpawnRunnerTest.java | 3 +- .../remote/RemoteExecutionServiceTest.java | 4 +- .../lib/remote/RemotePathResolverTest.java | 7 +- .../lib/remote/RemoteSpawnCacheTest.java | 3 +- .../util/FakeSpawnExecutionContext.java | 3 +- .../lib/sandbox/SpawnRunnerTestUtil.java | 3 +- .../bazel/remote/remote_execution_test.sh | 25 +++ 21 files changed, 234 insertions(+), 110 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 f7fd7b3ea6ca38..01ea75faba166f 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 @@ -185,7 +185,7 @@ public ImmutableList exec( spawnLogContext.logSpawn( spawn, actionExecutionContext.getMetadataProvider(), - context.getInputMapping(PathFragment.EMPTY_FRAGMENT), + context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ false), context.getTimeout(), spawnResult); } catch (IOException | ForbiddenActionInputException e) { @@ -246,7 +246,9 @@ public ListenableFuture prefetchInputs() return actionExecutionContext .getActionInputPrefetcher() .prefetchFiles( - getInputMapping(PathFragment.EMPTY_FRAGMENT).values(), getMetadataProvider()); + getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true) + .values(), + getMetadataProvider()); } return immediateVoidFuture(); @@ -306,22 +308,33 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping(PathFragment baseDirectory) + public SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException { - if (lazyInputMapping == null || !inputMappingBaseDirectory.equals(baseDirectory)) { - try (SilentCloseable c = - Profiler.instance().profile("AbstractSpawnStrategy.getInputMapping")) { - inputMappingBaseDirectory = baseDirectory; - lazyInputMapping = - spawnInputExpander.getInputMapping( - spawn, - actionExecutionContext.getArtifactExpander(), - baseDirectory, - actionExecutionContext.getMetadataProvider()); - } + // Return previously computed copy if present. + if (lazyInputMapping != null && inputMappingBaseDirectory.equals(baseDirectory)) { + return lazyInputMapping; + } + + SortedMap inputMapping; + try (SilentCloseable c = + Profiler.instance().profile("AbstractSpawnStrategy.getInputMapping")) { + inputMapping = + spawnInputExpander.getInputMapping( + spawn, + actionExecutionContext.getArtifactExpander(), + baseDirectory, + actionExecutionContext.getMetadataProvider()); } - return lazyInputMapping; + // Don't cache the input mapping if it is unlikely that it is used again. + // This reduces memory usage in the case where remote caching/execution is + // used, and the expected cache hit rate is high. + if (willAccessRepeatedly) { + inputMappingBaseDirectory = baseDirectory; + lazyInputMapping = inputMapping; + } + return inputMapping; } @Override 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 ee87201b1b1f79..ccfc3c8146043a 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 @@ -251,7 +251,8 @@ void lockOutputFiles(int exitCode, String errorMessage, FileOutErr outErr) * mapping is used in a context where the directory relative to which the keys are interpreted * is not the same as the execroot. */ - SortedMap getInputMapping(PathFragment baseDirectory) + SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException; /** Reports a progress update to the Spawn strategy. */ diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java index e680ea484d818c..5e5774832ee04e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteAction.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.util.SortedMap; +import javax.annotation.Nullable; /** A value class representing an action which can be executed remotely. */ public class RemoteAction { @@ -36,7 +37,9 @@ public class RemoteAction { private final SpawnExecutionContext spawnExecutionContext; private final RemoteActionExecutionContext remoteActionExecutionContext; private final RemotePathResolver remotePathResolver; - private final MerkleTree merkleTree; + @Nullable private final MerkleTree merkleTree; + private final long inputBytes; + private final long inputFiles; private final Digest commandHash; private final Command command; private final Action action; @@ -51,12 +54,15 @@ public class RemoteAction { Digest commandHash, Command command, Action action, - ActionKey actionKey) { + ActionKey actionKey, + boolean remoteDiscardMerkleTrees) { this.spawn = spawn; this.spawnExecutionContext = spawnExecutionContext; this.remoteActionExecutionContext = remoteActionExecutionContext; this.remotePathResolver = remotePathResolver; - this.merkleTree = merkleTree; + this.merkleTree = remoteDiscardMerkleTrees ? null : merkleTree; + this.inputBytes = merkleTree.getInputBytes(); + this.inputFiles = merkleTree.getInputFiles(); this.commandHash = commandHash; this.command = command; this.action = action; @@ -80,12 +86,12 @@ public Spawn getSpawn() { * Returns the sum of file sizes plus protobuf sizes used to represent the inputs of this action. */ public long getInputBytes() { - return merkleTree.getInputBytes(); + return inputBytes; } /** Returns the number of input files of this action. */ public long getInputFiles() { - return merkleTree.getInputFiles(); + return inputFiles; } /** Returns the id this is action. */ @@ -111,6 +117,7 @@ public Command getCommand() { return command; } + @Nullable public MerkleTree getMerkleTree() { return merkleTree; } @@ -119,9 +126,9 @@ public MerkleTree getMerkleTree() { * Returns a {@link SortedMap} which maps from input paths for remote action to {@link * ActionInput}. */ - public SortedMap getInputMap() + public SortedMap getInputMap(boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException { - return remotePathResolver.getInputMapping(spawnExecutionContext); + return remotePathResolver.getInputMapping(spawnExecutionContext, willAccessRepeatedly); } /** 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 ffa3871a369970..2f7099a1730fe5 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 @@ -142,6 +142,7 @@ import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.Executor; import java.util.concurrent.Phaser; +import java.util.concurrent.Semaphore; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Predicate; import javax.annotation.Nullable; @@ -377,7 +378,9 @@ private MerkleTree buildInputMerkleTree( } return MerkleTree.merge(subMerkleTrees, digestUtil); } else { - SortedMap inputMap = remotePathResolver.getInputMapping(context); + SortedMap inputMap = + remotePathResolver.getInputMapping( + context, /* willAccessRepeatedly= */ !remoteOptions.remoteDiscardMerkleTrees); if (!outputDirMap.isEmpty()) { // The map returned by getInputMapping is mutable, but must not be mutated here as it is // shared with all other strategies. @@ -436,63 +439,90 @@ private static ByteString buildSalt(Spawn spawn) { return null; } + /** + * Semaphore for limiting the concurrent number of Merkle tree input roots we compute and keep in + * memory. + * + *

When --jobs is set to a high value to let the remote execution service runs many actions in + * parallel, there is no point in letting the local system compute Merkle trees of input roots + * with the same amount of parallelism. Not only does this make Bazel feel sluggish and slow to + * respond to being interrupted, it causes it to exhaust memory. + * + *

As there is no point in letting Merkle tree input root computation use a higher concurrency + * than the number of CPUs in the system, use a semaphore to limit the concurrency of + * buildRemoteAction(). + */ + private final Semaphore remoteActionBuildingSemaphore = + new Semaphore(Runtime.getRuntime().availableProcessors(), true); + + @Nullable + private ToolSignature getToolSignature(Spawn spawn, SpawnExecutionContext context) + throws IOException, ExecException, InterruptedException { + return remoteOptions.markToolInputs + && Spawns.supportsWorkers(spawn) + && !spawn.getToolFiles().isEmpty() + ? computePersistentWorkerSignature(spawn, context) + : null; + } + /** Creates a new {@link RemoteAction} instance from spawn. */ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context) throws IOException, ExecException, ForbiddenActionInputException, InterruptedException { - ToolSignature toolSignature = - remoteOptions.markToolInputs - && Spawns.supportsWorkers(spawn) - && !spawn.getToolFiles().isEmpty() - ? computePersistentWorkerSignature(spawn, context) - : null; - final MerkleTree merkleTree = buildInputMerkleTree(spawn, context, toolSignature); - - // Get the remote platform properties. - Platform platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); - if (toolSignature != null) { - platform = - PlatformUtils.getPlatformProto( - spawn, remoteOptions, ImmutableMap.of("persistentWorkerKey", toolSignature.key)); - } else { - platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); - } - - Command command = - buildCommand( - spawn.getOutputFiles(), - spawn.getArguments(), - spawn.getEnvironment(), - platform, - remotePathResolver); - Digest commandHash = digestUtil.compute(command); - Action action = - Utils.buildAction( - commandHash, - merkleTree.getRootDigest(), - platform, - context.getTimeout(), - Spawns.mayBeCachedRemotely(spawn), - buildSalt(spawn)); - - ActionKey actionKey = digestUtil.computeActionKey(action); - - RequestMetadata metadata = - TracingMetadataUtils.buildMetadata( - buildRequestId, commandId, actionKey.getDigest().getHash(), spawn.getResourceOwner()); - RemoteActionExecutionContext remoteActionExecutionContext = - RemoteActionExecutionContext.create( - spawn, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn)); - - return new RemoteAction( - spawn, - context, - remoteActionExecutionContext, - remotePathResolver, - merkleTree, - commandHash, - command, - action, - actionKey); + remoteActionBuildingSemaphore.acquire(); + try { + ToolSignature toolSignature = getToolSignature(spawn, context); + final MerkleTree merkleTree = buildInputMerkleTree(spawn, context, toolSignature); + + // Get the remote platform properties. + Platform platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); + if (toolSignature != null) { + platform = + PlatformUtils.getPlatformProto( + spawn, remoteOptions, ImmutableMap.of("persistentWorkerKey", toolSignature.key)); + } else { + platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); + } + + Command command = + buildCommand( + spawn.getOutputFiles(), + spawn.getArguments(), + spawn.getEnvironment(), + platform, + remotePathResolver); + Digest commandHash = digestUtil.compute(command); + Action action = + Utils.buildAction( + commandHash, + merkleTree.getRootDigest(), + platform, + context.getTimeout(), + Spawns.mayBeCachedRemotely(spawn), + buildSalt(spawn)); + + ActionKey actionKey = digestUtil.computeActionKey(action); + + RequestMetadata metadata = + TracingMetadataUtils.buildMetadata( + buildRequestId, commandId, actionKey.getDigest().getHash(), spawn.getResourceOwner()); + RemoteActionExecutionContext remoteActionExecutionContext = + RemoteActionExecutionContext.create( + spawn, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn)); + + return new RemoteAction( + spawn, + context, + remoteActionExecutionContext, + remotePathResolver, + merkleTree, + commandHash, + command, + action, + actionKey, + remoteOptions.remoteDiscardMerkleTrees); + } finally { + remoteActionBuildingSemaphore.release(); + } } @Nullable @@ -1338,7 +1368,7 @@ private void reportUploadError(Throwable error) { *

Must be called before calling {@link #executeRemotely}. */ public void uploadInputsIfNotPresent(RemoteAction action, boolean force) - throws IOException, InterruptedException { + throws IOException, ExecException, ForbiddenActionInputException, InterruptedException { checkState(!shutdown.get(), "shutdown"); checkState(mayBeExecutedRemotely(action.getSpawn()), "spawn can't be executed remotely"); @@ -1347,13 +1377,33 @@ public void uploadInputsIfNotPresent(RemoteAction action, boolean force) Map additionalInputs = Maps.newHashMapWithExpectedSize(2); additionalInputs.put(action.getActionKey().getDigest(), action.getAction()); additionalInputs.put(action.getCommandHash(), action.getCommand()); - remoteExecutionCache.ensureInputsPresent( - action - .getRemoteActionExecutionContext() - .withWriteCachePolicy(CachePolicy.REMOTE_CACHE_ONLY), // Only upload to remote cache - action.getMerkleTree(), - additionalInputs, - force); + + // As uploading depends on having the full input root in memory, limit + // concurrency. This prevents memory exhaustion. We assume that + // ensureInputsPresent() provides enough parallelism to saturate the + // network connection. + remoteActionBuildingSemaphore.acquire(); + try { + MerkleTree merkleTree = action.getMerkleTree(); + if (merkleTree == null) { + // --experimental_remote_discard_merkle_trees was provided. + // Recompute the input root. + Spawn spawn = action.getSpawn(); + SpawnExecutionContext context = action.getSpawnExecutionContext(); + ToolSignature toolSignature = getToolSignature(spawn, context); + merkleTree = buildInputMerkleTree(spawn, context, toolSignature); + } + + remoteExecutionCache.ensureInputsPresent( + action + .getRemoteActionExecutionContext() + .withWriteCachePolicy(CachePolicy.REMOTE_CACHE_ONLY), // Only upload to remote cache + merkleTree, + additionalInputs, + force); + } finally { + remoteActionBuildingSemaphore.release(); + } } /** 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 8da2b2b0455411..09d821a0108e11 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 @@ -202,7 +202,7 @@ public void close() {} private void checkForConcurrentModifications() throws IOException, ForbiddenActionInputException { - for (ActionInput input : action.getInputMap().values()) { + for (ActionInput input : action.getInputMap(true).values()) { if (input instanceof VirtualActionInput) { continue; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index aa2557d3770dd5..49f2b6b73295ae 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -621,9 +621,9 @@ private Map getInputCtimes(SortedMap inpu SpawnResult execLocallyAndUpload( RemoteAction action, Spawn spawn, SpawnExecutionContext context, boolean uploadLocalResults) throws ExecException, IOException, ForbiddenActionInputException, InterruptedException { - Map ctimesBefore = getInputCtimes(action.getInputMap()); + Map ctimesBefore = getInputCtimes(action.getInputMap(true)); SpawnResult result = execLocally(spawn, context); - Map ctimesAfter = getInputCtimes(action.getInputMap()); + Map ctimesAfter = getInputCtimes(action.getInputMap(true)); uploadLocalResults = uploadLocalResults && Status.SUCCESS.equals(result.status()) && result.exitCode() == 0; if (!uploadLocalResults) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java index d2c1c2c5448a38..5b2bde1377ef60 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java @@ -42,7 +42,8 @@ public interface RemotePathResolver { * Returns a {@link SortedMap} which maps from input paths for remote action to {@link * ActionInput}. */ - SortedMap getInputMapping(SpawnExecutionContext context) + SortedMap getInputMapping( + SpawnExecutionContext context, boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException; void walkInputs( @@ -100,9 +101,10 @@ public String getWorkingDirectory() { } @Override - public SortedMap getInputMapping(SpawnExecutionContext context) + public SortedMap getInputMapping( + SpawnExecutionContext context, boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException { - return context.getInputMapping(PathFragment.EMPTY_FRAGMENT); + return context.getInputMapping(PathFragment.EMPTY_FRAGMENT, willAccessRepeatedly); } @Override @@ -171,12 +173,14 @@ public String getWorkingDirectory() { } @Override - public SortedMap getInputMapping(SpawnExecutionContext context) + public SortedMap getInputMapping( + SpawnExecutionContext context, boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException { // The "root directory" of the action from the point of view of RBE is the parent directory of // the execroot locally. This is so that paths of artifacts in external repositories don't // start with an uplevel reference. - return context.getInputMapping(PathFragment.create(checkNotNull(getWorkingDirectory()))); + return context.getInputMapping( + PathFragment.create(checkNotNull(getWorkingDirectory())), willAccessRepeatedly); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 0fc54e09557313..c812a423aa3f77 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -646,6 +646,18 @@ public RemoteOutputsStrategyConverter() { + "can be used to implement remote persistent workers.") public boolean markToolInputs; + @Option( + name = "experimental_remote_discard_merkle_trees", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If set to true, discard in-memory copies of the input root's Merkle tree and associated " + + "input mappings during calls to GetActionResult() and Execute(). This reduces " + + "memory usage significantly, but does require Bazel to recompute them upon remote " + + "cache misses and retries.") + public boolean remoteDiscardMerkleTrees; + // The below options are not configurable by users, only tests. // This is part of the effort to reduce the overall number of flags. diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java index 630a4551485e7f..6145855522621d 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java @@ -232,7 +232,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( - context.getInputMapping(PathFragment.EMPTY_FRAGMENT), + context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java index 1a46186e76c991..68490bf7739ecd 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java @@ -222,7 +222,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( - context.getInputMapping(PathFragment.EMPTY_FRAGMENT), + context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java index 89eb4a70f0668f..a50f4ba89f48be 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java @@ -218,7 +218,9 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context ImmutableSet writableDirs = getWritableDirs(sandboxExecRoot, environment); SandboxInputs inputs = - helpers.processInputFiles(context.getInputMapping(PathFragment.EMPTY_FRAGMENT), execRoot); + helpers.processInputFiles( + context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); Duration timeout = context.getTimeout(); @@ -437,7 +439,10 @@ public void verifyPostCondition( private void checkForConcurrentModifications(SpawnExecutionContext context) throws IOException, ForbiddenActionInputException { - for (ActionInput input : context.getInputMapping(PathFragment.EMPTY_FRAGMENT).values()) { + for (ActionInput input : + context + .getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true) + .values()) { if (input instanceof VirtualActionInput) { // Virtual inputs are not existing in file system and can't be tampered with via sandbox. No // need to check them. diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java index 6b6835eff2b549..5f2a2442f8b0b4 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java @@ -111,7 +111,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( - context.getInputMapping(PathFragment.EMPTY_FRAGMENT), + context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java index 467aa6e5c868ae..7b200365bd972a 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java @@ -71,7 +71,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs readablePaths = helpers.processInputFiles( - context.getInputMapping(PathFragment.EMPTY_FRAGMENT), + context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), execRoot); ImmutableSet.Builder writablePaths = ImmutableSet.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index 7b82c7a94236c8..0f7097993a4745 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -188,7 +188,9 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) Profiler.instance().profile(ProfilerTask.WORKER_SETUP, "Setting up inputs")) { inputFiles = helpers.processInputFiles( - context.getInputMapping(PathFragment.EMPTY_FRAGMENT), execRoot); + context.getInputMapping( + PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), + execRoot); } SandboxOutputs outputs = helpers.getOutputs(spawn); 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 8dfd86a1ddac49..46f6e704dbfc22 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 @@ -284,7 +284,8 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping(PathFragment baseDirectory) { + public SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) { return inputMapping; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 1808b86d66c5ef..5145d78b216e4c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1698,10 +1698,10 @@ public void uploadInputsIfNotPresent_interrupted_requestCancelled() throws Excep new Thread( () -> { try { - service.uploadInputsIfNotPresent(action, /*force=*/ false); + service.uploadInputsIfNotPresent(action, /* force= */ false); } catch (InterruptedException ignored) { interrupted.countDown(); - } catch (IOException ignored) { + } catch (Exception ignored) { // intentionally ignored } }); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemotePathResolverTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemotePathResolverTest.java index 3805649a1e0bab..0d36c1ec0bf2fb 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemotePathResolverTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemotePathResolverTest.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -50,7 +51,7 @@ public void setup() throws Exception { input = ActionInputHelper.fromPath("foo"); spawnExecutionContext = mock(SpawnExecutionContext.class); - when(spawnExecutionContext.getInputMapping(any())) + when(spawnExecutionContext.getInputMapping(any(), anyBoolean())) .thenAnswer( invocationOnMock -> { PathFragment baseDirectory = invocationOnMock.getArgument(0); @@ -83,7 +84,7 @@ public void getInputMapping_default_inputsRelativeToExecRoot() throws Exception RemotePathResolver remotePathResolver = RemotePathResolver.createDefault(execRoot); SortedMap inputs = - remotePathResolver.getInputMapping(spawnExecutionContext); + remotePathResolver.getInputMapping(spawnExecutionContext, false); assertThat(inputs).containsExactly(PathFragment.create("foo"), input); } @@ -93,7 +94,7 @@ public void getInputMapping_sibling_inputsRelativeToInputRoot() throws Exception RemotePathResolver remotePathResolver = new SiblingRepositoryLayoutResolver(execRoot); SortedMap inputs = - remotePathResolver.getInputMapping(spawnExecutionContext); + remotePathResolver.getInputMapping(spawnExecutionContext, false); assertThat(inputs).containsExactly(PathFragment.create("main/foo"), input); } 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 59f8b8ad4bc1cf..05a635dfde09ae 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 @@ -172,7 +172,8 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping(PathFragment baseDirectory) + public SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException { return getSpawnInputExpander() .getInputMapping(simpleSpawn, SIMPLE_ARTIFACT_EXPANDER, baseDirectory, fakeFileCache); diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java index 3c25136a3b5509..46dc2572a29ae9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java @@ -133,7 +133,8 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping(PathFragment baseDirectory) + public SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) throws IOException, ForbiddenActionInputException { return getSpawnInputExpander() .getInputMapping(spawn, this::artifactExpander, baseDirectory, metadataProvider); diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java b/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java index 562d7eabb90aa0..420721c2b30c31 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java @@ -131,7 +131,8 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping(PathFragment baseDirectory) { + public SortedMap getInputMapping( + PathFragment baseDirectory, boolean willAccessRepeatedly) { TreeMap inputMapping = new TreeMap<>(); for (ActionInput actionInput : spawn.getInputFiles().toList()) { inputMapping.put(baseDirectory.getRelative(actionInput.getExecPath()), actionInput); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 6d6de17de93479..6f66889aa67c86 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2199,6 +2199,13 @@ function test_empty_tree_artifact_as_inputs() { --experimental_remote_merkle_tree_cache \ //pkg:a &>$TEST_log || fail "expected build to succeed with Merkle tree cache" + bazel clean --expunge + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_remote_discard_merkle_trees \ + //pkg:a &>$TEST_log || fail "expected build to succeed with Merkle tree discarding" + bazel clean --expunge bazel build \ --spawn_strategy=remote \ @@ -2273,6 +2280,15 @@ function test_create_tree_artifact_outputs() { [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file" [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist" + bazel clean --expunge + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_remote_discard_merkle_trees \ + //pkg:a &>$TEST_log || fail "expected build to succeed with Merkle tree discarding" + [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file" + [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist" + bazel clean --expunge bazel build \ --spawn_strategy=remote \ @@ -3099,6 +3115,15 @@ EOF --experimental_remote_merkle_tree_cache \ //pkg:b &>$TEST_log || fail "expected build to succeed with Merkle tree cache" + bazel clean --expunge + bazel \ + --windows_enable_symlinks \ + build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_remote_discard_merkle_trees \ + //pkg:b &>$TEST_log || fail "expected build to succeed with Merkle tree discarding" + if [[ "$(cat bazel-bin/pkg/b.txt)" != "$link_target" ]]; then fail "expected symlink target to be $link_target" fi