Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop stashing tmp directories for tests #21412

Closed
wants to merge 2 commits into from
Closed

Stop stashing tmp directories for tests #21412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
019929f
Stop stashing tmp directories for tests
oquenchil Feb 19, 2024
91d1d99
Remove unneeded synchronized block
oquenchil Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/main/java/com/google/devtools/build/lib/sandbox/SandboxStash.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,32 +119,33 @@ private boolean takeStashedSandboxInternal(

/** Atomically moves the sandboxPath directory aside for later reuse. */
static void stashSandbox(
Path path, String mnemonic, Map<String, String> environment, SandboxOutputs outputs) {
Path path, String mnemonic, Map<String, String> environment, SandboxOutputs outputs, TreeDeleter treeDeleter) {
if (instance == null) {
return;
}
instance.stashSandboxInternal(path, mnemonic, environment, outputs);
instance.stashSandboxInternal(path, mnemonic, environment, outputs, treeDeleter);
}

private void stashSandboxInternal(
Path path, String mnemonic, Map<String, String> environment, SandboxOutputs outputs) {
Path path, String mnemonic, Map<String, String> environment, SandboxOutputs outputs, TreeDeleter treeDeleter) {
Path sandboxes = getSandboxStashDir(mnemonic, path.getFileSystem());
if (sandboxes == null || isTestXmlGenerationOrCoverageSpawn(mnemonic, outputs)) {
return;
}
String stashName;
synchronized (stash) {
stashName = Integer.toString(stash.incrementAndGet());
}
String stashName = Integer.toString(stash.incrementAndGet());
Path stashPath = sandboxes.getChild(stashName);
if (!path.exists()) {
return;
}
try {
stashPath.createDirectory();
Path stashPathExecroot = stashPath.getChild("execroot");
if (isTestAction(mnemonic)) {
treeDeleter.deleteTree(path.getRelative("execroot/" + environment.get("TEST_WORKSPACE") + "/_tmp"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it so that the containing method may now perform relatively costly IO. Is this clearly fine for the callers, for example because they would clean out the entire sandbox if it weren't reused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell this would indeed get more expensive for the caller here when async deletion is disabled (when it's enabled, there's no significant cost). It also doesn't mean that sync deletion had no cost before this change, it was just paid somewhere else when cleaning up that directory during reuse.

I don't think this matters much but please correct me if I'm wrong.

}
path.getChild("execroot").renameTo(stashPathExecroot);
if (isTestAction(mnemonic)) {
// We only do this after the rename operation has succeeded
Copy link
Collaborator

@fmeum fmeum Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to a change made in this PR, but the usage of synchronized in line 136 looks unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

stashPathToRunfilesDir.put(stashPathExecroot, getCurrentRunfilesDir(environment));
}
} catch (IOException e) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ protected void copyFile(Path source, Path target) throws IOException {

@Override
public void delete() {
SandboxStash.stashSandbox(sandboxPath, mnemonic, getEnvironment(), outputs);
SandboxStash.stashSandbox(sandboxPath, mnemonic, getEnvironment(), outputs, treeDeleter);
super.delete();
}

Expand Down
9 changes: 8 additions & 1 deletion src/test/shell/integration/sandboxing_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,7 @@ EOF
bazel shutdown
}

function test_runfiles_from_tests_get_reused() {
function test_runfiles_from_tests_get_reused_and_tmp_clean() {
mkdir pkg
touch pkg/file.txt
cat >pkg/reusing_test.bzl <<'EOF'
Expand Down Expand Up @@ -1013,6 +1013,13 @@ EOF
dir_inode_a=$(awk '/The directory inode is/ {print $5}' ${test_output})
file_inode_a=$(awk '/The file inode is/ {print $5}' ${test_output})

local output_base="$(bazel info output_base)"
local stashed_test_dir="${output_base}/sandbox/sandbox_stash/TestRunner/6/execroot/$WORKSPACE_NAME"
[[ -d "${stashed_test_dir}/bazel-out" ]] \
|| fail "${stashed_test_dir}/bazel-out directory not present"
[[ -d "${stashed_test_dir}/_tmp" ]] \
&& fail "${stashed_test_dir}/_tmp directory is present"

if is_bazel; then
bazel coverage --test_output=streamed //pkg:b \
--experimental_split_coverage_postprocessing=1 \
Expand Down
Loading