Add option to track sandbox stashes in memory #22559

oquenchil · 2024-05-27T20:46:55Z

This change introduces the flag --experimental_inmemory_sandbox_stashes to track in memory the contents of the stashes stored with --reuse_sandbox_directories=true

With the old behavior Bazel has to perform a lot of I/O to read the contents of each stash before reusing it in a new action. Essentially, it checks every directory and subdirectory in the stashed sandbox to find out which files are different to the inputs of the new action about to be executed.

With in-memory stashes we associate each stash to the symlinks that needed to be created for that action. We also store the time at which these symlinks were created. In a background thread after the action has finished executing we stat every directory and for the ones that have changed (this should be rare) we update the contents. Because we only read the contents of the directories that have changed we do much less I/O than before.

If an action purposefully changes a sandbox symlink in-place, this won't be detected by statting the directory. I don't know any use case for this since the symlink itself is an implementation detail to achieve sandboxing. For this reason, manipulation of sandbox symlinks in-place is not supported.

Depending on the build this change might have a significant effect on memory. It should generally improve wall time further in builds where --reuse_sandbox_directories already improved wall time.

This change also introduces a minor optimization which is to associate each stash with the target that it was originally created for. When a new action wants to reuse a stash and there is more than one available, it will take the stash whose target is closest to its own. This is done with the assumption that targets that are closer together in the graph will have more inputs in common.

Fixes #22309 .

Closes bazelbuild#22044. PiperOrigin-RevId: 625934206 Change-Id: Iee7e8fa9435f6b027755890e3a8be3ac2025e066

…ontents

In memory reuse sandbox directory stashes Closes bazelbuild#22559. PiperOrigin-RevId: 638617294 Change-Id: I021343a1e5411734f7b09d941092620191619ade

fmeum · 2024-05-30T15:46:44Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

+
+  /** Used to store sandbox stashes in-memory. */
+  @AutoValue
+  public abstract static class StashContents {


This would be more concise as a record.

fmeum · 2024-05-30T16:00:24Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java

-  public SandboxStash(String workspaceName, Path sandboxBase) {
+  private static final int POOL_SIZE = Runtime.getRuntime().availableProcessors();
+  private final ExecutorService stashFileListingPool =
+      Executors.newFixedThreadPool(


Have you tried/considered using virtual threads here? This looks mostly IO-bound.

I will wait for that till we check in proper common code for I/O operations using virtual threads. As far as I understand, this requires enough careful consideration to be outside the scope of this change and the number of new Java threads I'm introducing here is not very significant compared to how many we already use anyway.

Let me copy paste part of what @coeuvre has written in the past (there isn't any public document) about this:

The vast majority of blocking operations in the JDK will unmount the virtual thread, freeing its carrier and the underlying OS thread to take on new work. However, some blocking operations in the JDK do not unmount the virtual thread, and thus block both its carrier and the underlying OS thread due to OS level limitations (e.g. many filesystem operations). The implementation of these blocking operations will compensate for the capture of the OS thread by temporarily expanding the parallelism of the scheduler. Consequently, the number of platform threads in the scheduler's thread pool may temporarily exceed the number of available cores.

For Bazel, we use VFS and do filesystem calls with JNI, bypassing file APIs from JDK. This means we need to implement similar compensation in VFS as JDK did for its file API for better performance.

Makes sense to do this in a follow-up change (although I think that the compensation mechanism has been implemented for vfs).

fmeum · 2024-06-01T16:47:12Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

+  @AutoValue
+  public abstract static class StashContents {
+    @SuppressWarnings("AutoValueImmutableFields")
+    public abstract Map<String, Path> filesToPath();


Could you add a comment to explain what the keys are (individual path segments)?

fmeum · 2024-06-01T16:50:50Z

src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java

+        // `sandboxExecRoot`. This will use what we computed above, delete anything unnecessary, and
+        // update `inputsToCreate`/`dirsToCreate` if something can be left without changes (e.g., a,
+        // symlink that already points to the right destination). We're traversing from
+        // sandboxExecRoot's parent directory because external repositories can now be symlinked as


This part applies to the other branch as well.

fmeum · 2024-06-01T16:51:37Z

src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java

+            stashContents.get());
+      } else {
+        // No in-memory stashes enabled but there is a stash.
+        // When reusing an old sandbox, we do a full traversal of the parent directory of


Would we avoid code complexity if we just didn't create in-memory stashes for existing stashes?

I don't know exactly what you mean. I will answer two different questions that might not be what you meant.

Can we create only the stash in the background thread and compare there as opposed to creating a stash here in this function and then doing it again in the background thread with the actual content?
That would be less efficient since in this function we are not readdirring the directories, only adding to the stash the declared inputs and outputs. Then in the background thread we are only changing the dirs that have been modified

Can we stop creating in-memory stashes for existing disk stashes that don't have a stashContents yet?
We may have a disk stash without a corresponding stashContents if during the lifetime of the server we only enable the flag in a later command.

Please let me know if the question wasn't any of those.

The latter. I was wondering whether dropping all stashes when flipping the flag instead of scanning them would make the code simpler. But it also looks fine as is.

fmeum · 2024-06-01T16:52:37Z

src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java

+    }
+
+    if (SandboxStash.useInMemoryStashes()) {
+      Map<PathFragment, StashContents> stashContentsMap = new HashMap<>();


If peak memory is a concern, we could potentially look into CompactHashMap.

Done. I made the StashContents members use CompactHashMap too.

fmeum · 2024-06-01T16:56:42Z

src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java

+      }
+
+      for (var outputDir :
+          Iterables.concat(


If you use Stream.concat instead, you could also add distinct() to the chain. I don't yet know whether this would result in duplicate work, will keep reading.

fmeum · 2024-06-01T20:42:13Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java

+      stashContents
+          .symlinksToPathFragment()
+          .keySet()
+          .removeAll(


Could this be replaced with retainAll?

fmeum · 2024-06-01T20:47:42Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java

+        Comparator.comparingInt(countMap::get).reversed(), sortedStashes);
+  }
+
+  private int getMatchingSegmentsCount(String[] target, String[] oldTarget) {


I think that this could be replaced by Arrays.mismatch.

fmeum · 2024-06-01T20:48:28Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java

+    for (Path stash : stashes) {
+      Label stashTarget = sandboxToTarget.getOrDefault(stash, /* defaultValue= */ null);
+      if (stashTarget == null) {
+        sortedStashes.remove(stash);


If these stashes are never reused, could we just drop them right away?

With the current logic they might be reused. If the current target is also null, we just return the unsorted list which will contain other stashes without an associated target. If the first one available happens to have no associated target, it gets reused.

But I changed the logic now a bit so that if the current action has a null target it will try to use any others with a null target first.

fmeum · 2024-06-01T20:52:50Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java

+    if (target == null) {
+      return stashes;
+    }
+    List<Path> sortedStashes = new ArrayList<>(stashes);


I don't know whether this sorting is a bottleneck, but if profiling shows it is, we could keep the stashes in a TreeMap by PackageIdentifier and walk up and down from the insertion position of the new entry to discover the closest stashes.

Since PackageIdentifier already implements Comparable, maybe that's even easier than the current logic?

I don't think this is important. There will never be more than 2*n_cores stashes, if there are then there is a different problem somewhere. At the same time, I don't think the resulting code would be significantly simpler.

fmeum · 2024-06-01T20:57:33Z

src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java

+    String segment = stashedRunfiles.get(i);
+    Preconditions.checkState(stashContents.dirEntries().containsKey(segment));
+    if (i < stashedRunfiles.size() - 1) {
+      return getStashedRunfilesStashContents(


I didn't quite get what this function is doing. Is it calling itself recursively in what's essentially a loop? If so, why can't this just be a loop?

oquenchil · 2024-06-04T11:10:10Z

Thank you very much for the great review!

This change introduces the flag `--experimental_inmemory_sandbox_stashes` to track in memory the contents of the stashes stored with `--reuse_sandbox_directories=true` With the old behavior Bazel has to perform a lot of I/O to read the contents of each stash before reusing it in a new action. Essentially, it checks every directory and subdirectory in the stashed sandbox to find out which files are different to the inputs of the new action about to be executed. With in-memory stashes we associate each stash to the symlinks that needed to be created for that action. We also store the time at which these symlinks were created. In a background thread after the action has finished executing we stat every directory and for the ones that have changed (this should be rare) we update the contents. Because we only read the contents of the directories that have changed we do much less I/O than before. If an action purposefully changes a sandbox symlink in-place, this won't be detected by statting the directory. I don't know any use case for this since the symlink itself is an implementation detail to achieve sandboxing. For this reason, manipulation of sandbox symlinks in-place is not supported. Depending on the build this change might have a significant effect on memory. It should generally improve wall time further in builds where `--reuse_sandbox_directories` already improved wall time. This change also introduces a minor optimization which is to associate each stash with the target that it was originally created for. When a new action wants to reuse a stash and there is more than one available, it will take the stash whose target is closest to its own. This is done with the assumption that targets that are closer together in the graph will have more inputs in common. Fixes #22309 . Closes #22559. PiperOrigin-RevId: 640142481 Change-Id: Iece2d718df47f403e2fe91c1bd887604eceee8ee Commit 1c0135c Co-authored-by: Pedro Liberal Fernndez <plf@google.com>

oquenchil added 14 commits May 17, 2024 03:41

Out of critical path stashing

3663bfd

Closes bazelbuild#22044. PiperOrigin-RevId: 625934206 Change-Id: Iee7e8fa9435f6b027755890e3a8be3ac2025e066

Fix runfiles optimization

a4a78d1

Fix bug with deleted StashContents dirEntry

7b603ba

Remove code repetition and guard behind flag

7776ea2

Merge branch 'master' of github.com:bazelbuild/bazel into memory_stashes

392efb6

Add more logging for mismatched runfile path lengths.

ee326c2

Fix package path not taken into account when updating runfiles StashC…

8690c1e

…ontents

Add test for in-memory stashes

b0d8a6b

Fix failing tests due to null target label

4d3a7f2

Cloned from CL 638605923 by 'g4 patch'.

a584708

In memory reuse sandbox directory stashes Closes bazelbuild#22559. PiperOrigin-RevId: 638617294 Change-Id: I021343a1e5411734f7b09d941092620191619ade

Merge branch 'master' of github.com:bazelbuild/bazel into memory_stashes

c5051b5

Fix lint errors

7c3684c

Change equals method

faefe7b

Fix test relying on tree utility which might not be present

7cfb5c9

oquenchil changed the title ~~In memory reuse sandbox directory stashes~~ Add option to track sandbox stashes in memory May 30, 2024

oquenchil requested a review from fmeum May 30, 2024 14:48

oquenchil marked this pull request as ready for review May 30, 2024 14:49

github-actions bot added team-Local-Exec Issues and PRs for the Execution (Local) team awaiting-review PR is awaiting review from an assigned reviewer labels May 30, 2024

fmeum reviewed May 30, 2024

View reviewed changes

fmeum reviewed Jun 1, 2024

View reviewed changes

Review comments and macOS test fix

9485acc

fmeum approved these changes Jun 4, 2024

View reviewed changes

copybara-service bot closed this in 1c0135c Jun 4, 2024

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 4, 2024

keertk mentioned this pull request Jun 5, 2024

[7.2.0] Add option to track sandbox stashes in memory #22637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to track sandbox stashes in memory #22559

Add option to track sandbox stashes in memory #22559

oquenchil commented May 27, 2024 •

edited

Loading

fmeum May 30, 2024

oquenchil Jun 3, 2024

fmeum May 30, 2024

oquenchil Jun 3, 2024

fmeum Jun 4, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 4, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

fmeum Jun 1, 2024

oquenchil Jun 3, 2024

oquenchil commented Jun 4, 2024

Add option to track sandbox stashes in memory #22559

Add option to track sandbox stashes in memory #22559

Conversation

oquenchil commented May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oquenchil commented Jun 4, 2024

oquenchil commented May 27, 2024 •

edited

Loading