Skip to content

Commit 31db460

Browse files
larsrc-googlecopybara-github
larsrc-google
authored and
copybara-github
committed
Make WorkerExecRoot not be re-created on each createFileSystem() call. Preparation for holding a map of existing links, but also just nicer.
RELNOTES: None PiperOrigin-RevId: 356465646
1 parent 4b68532 commit 31db460

File tree

7 files changed

+88
-91
lines changed

7 files changed

+88
-91
lines changed

‎src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,31 +25,27 @@
2525
/** A {@link SingleplexWorker} that runs inside a sandboxed execution root. */
2626
final class SandboxedWorker extends SingleplexWorker {
2727
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
28-
private WorkerExecRoot workerExecRoot;
28+
private final WorkerExecRoot workerExecRoot;
2929

3030
SandboxedWorker(WorkerKey workerKey, int workerId, Path workDir, Path logFile) {
3131
super(workerKey, workerId, workDir, logFile);
32+
workerExecRoot = new WorkerExecRoot(workDir);
3233
}
3334

3435
@Override
3536
public void prepareExecution(
3637
SandboxInputs inputFiles, SandboxOutputs outputs, Set<PathFragment> workerFiles)
3738
throws IOException {
38-
// Note that workerExecRoot isn't necessarily null at this point, so we can't do a Preconditions
39-
// check for it: If a WorkerSpawnStrategy gets interrupted, finishExecution is not guaranteed to
40-
// be called.
41-
workerExecRoot = new WorkerExecRoot(workDir, inputFiles, outputs, workerFiles);
42-
workerExecRoot.createFileSystem();
39+
workerExecRoot.createFileSystem(workerFiles, inputFiles, outputs);
4340

4441
super.prepareExecution(inputFiles, outputs, workerFiles);
4542
}
4643

4744
@Override
48-
public void finishExecution(Path execRoot) throws IOException {
49-
super.finishExecution(execRoot);
45+
public void finishExecution(Path execRoot, SandboxOutputs outputs) throws IOException {
46+
super.finishExecution(execRoot, outputs);
5047

51-
workerExecRoot.copyOutputs(execRoot);
52-
workerExecRoot = null;
48+
workerExecRoot.copyOutputs(execRoot, outputs);
5349
}
5450

5551
@Override

‎src/main/java/com/google/devtools/build/lib/worker/SingleplexWorker.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ WorkResponse getResponse(int requestId) throws IOException {
124124
}
125125

126126
@Override
127-
public void finishExecution(Path execRoot) throws IOException {}
127+
public void finishExecution(Path execRoot, SandboxOutputs outputs) throws IOException {}
128128

129129
@Override
130130
void destroy() {

‎src/main/java/com/google/devtools/build/lib/worker/Worker.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ public abstract void prepareExecution(
101101
abstract WorkResponse getResponse(int requestId) throws IOException, InterruptedException;
102102

103103
/** Does whatever cleanup may be required after execution is done. */
104-
public abstract void finishExecution(Path execRoot) throws IOException;
104+
public abstract void finishExecution(Path execRoot, SandboxOutputs outputs) throws IOException;
105105

106106
/**
107107
* Destroys this worker. Once this has been called, we assume it's safe to clean up related

‎src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -30,42 +30,41 @@
3030
/** Creates and manages the contents of a working directory of a persistent worker. */
3131
final class WorkerExecRoot {
3232
private final Path workDir;
33-
private final SandboxInputs inputs;
34-
private final SandboxOutputs outputs;
35-
private final Set<PathFragment> workerFiles;
3633

37-
public WorkerExecRoot(
38-
Path workDir, SandboxInputs inputs, SandboxOutputs outputs, Set<PathFragment> workerFiles) {
34+
public WorkerExecRoot(Path workDir) {
3935
this.workDir = workDir;
40-
this.inputs = inputs;
41-
this.outputs = outputs;
42-
this.workerFiles = workerFiles;
4336
}
4437

45-
public void createFileSystem() throws IOException {
38+
public void createFileSystem(
39+
Set<PathFragment> workerFiles, SandboxInputs inputs, SandboxOutputs outputs)
40+
throws IOException {
4641
workDir.createDirectoryAndParents();
4742

4843
// First compute all the inputs and directories that we need. This is based only on
4944
// `workerFiles`, `inputs` and `outputs` and won't do any I/O.
5045
Set<PathFragment> inputsToCreate = new LinkedHashSet<>();
5146
LinkedHashSet<PathFragment> dirsToCreate = new LinkedHashSet<>();
52-
populateInputsAndDirsToCreate(inputsToCreate, dirsToCreate);
47+
populateInputsAndDirsToCreate(inputs, workerFiles, outputs, inputsToCreate, dirsToCreate);
5348

5449
// Then do a full traversal of the `workDir`. This will use what we computed above, delete
5550
// anything unnecessary and update `inputsToCreate`/`dirsToCreate` if something is can be left
5651
// without changes (e.g., a symlink that already points to the right destination).
57-
cleanExisting(workDir, inputsToCreate, dirsToCreate);
52+
cleanExisting(workDir, inputs, inputsToCreate, dirsToCreate);
5853

5954
// Finally, create anything that is still missing.
6055
createDirectories(dirsToCreate);
61-
createInputs(inputsToCreate);
56+
createInputs(inputsToCreate, inputs);
6257

6358
inputs.materializeVirtualInputs(workDir);
6459
}
6560

6661
/** Populates the provided sets with the inputs and directories than need to be created. */
6762
private void populateInputsAndDirsToCreate(
68-
Set<PathFragment> inputsToCreate, LinkedHashSet<PathFragment> dirsToCreate) {
63+
SandboxInputs inputs,
64+
Set<PathFragment> workerFiles,
65+
SandboxOutputs outputs,
66+
Set<PathFragment> inputsToCreate,
67+
LinkedHashSet<PathFragment> dirsToCreate) {
6968
// Add all worker files and the ancestor directories.
7069
for (PathFragment path : workerFiles) {
7170
inputsToCreate.add(path);
@@ -101,12 +100,16 @@ private void populateInputsAndDirsToCreate(
101100
* correct and doesn't need any changes.
102101
*/
103102
private void cleanExisting(
104-
Path root, Set<PathFragment> inputsToCreate, Set<PathFragment> dirsToCreate)
103+
Path root,
104+
SandboxInputs inputs,
105+
Set<PathFragment> inputsToCreate,
106+
Set<PathFragment> dirsToCreate)
105107
throws IOException {
106108
for (Path path : root.getDirectoryEntries()) {
107109
FileStatus stat = path.stat(Symlinks.NOFOLLOW);
108110
PathFragment pathRelativeToWorkDir = path.relativeTo(workDir);
109-
Optional<PathFragment> destination = getExpectedSymlinkDestination(pathRelativeToWorkDir);
111+
Optional<PathFragment> destination =
112+
getExpectedSymlinkDestination(pathRelativeToWorkDir, inputs);
110113
if (destination.isPresent()) {
111114
if (stat.isSymbolicLink() && path.readSymbolicLink().equals(destination.get())) {
112115
inputsToCreate.remove(pathRelativeToWorkDir);
@@ -115,7 +118,7 @@ private void cleanExisting(
115118
}
116119
} else if (stat.isDirectory()) {
117120
if (dirsToCreate.contains(pathRelativeToWorkDir)) {
118-
cleanExisting(path, inputsToCreate, dirsToCreate);
121+
cleanExisting(path, inputs, inputsToCreate, dirsToCreate);
119122
dirsToCreate.remove(pathRelativeToWorkDir);
120123
} else {
121124
path.deleteTree();
@@ -126,7 +129,8 @@ private void cleanExisting(
126129
}
127130
}
128131

129-
private Optional<PathFragment> getExpectedSymlinkDestination(PathFragment fragment) {
132+
private Optional<PathFragment> getExpectedSymlinkDestination(
133+
PathFragment fragment, SandboxInputs inputs) {
130134
Path file = inputs.getFiles().get(fragment);
131135
if (file != null) {
132136
return Optional.of(file.asFragment());
@@ -140,7 +144,8 @@ private void createDirectories(Iterable<PathFragment> dirsToCreate) throws IOExc
140144
}
141145
}
142146

143-
private void createInputs(Iterable<PathFragment> inputsToCreate) throws IOException {
147+
private void createInputs(Iterable<PathFragment> inputsToCreate, SandboxInputs inputs)
148+
throws IOException {
144149
for (PathFragment fragment : inputsToCreate) {
145150
Path key = workDir.getRelative(fragment);
146151
if (inputs.getFiles().containsKey(fragment)) {
@@ -159,7 +164,7 @@ private void createInputs(Iterable<PathFragment> inputsToCreate) throws IOExcept
159164
}
160165
}
161166

162-
public void copyOutputs(Path execRoot) throws IOException {
167+
public void copyOutputs(Path execRoot, SandboxOutputs outputs) throws IOException {
163168
SandboxHelpers.moveOutputs(outputs, workDir, execRoot);
164169
}
165170
}

‎src/main/java/com/google/devtools/build/lib/worker/WorkerProxy.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ WorkResponse getResponse(int requestId) throws InterruptedException {
9494
}
9595

9696
@Override
97-
public void finishExecution(Path execRoot) {}
97+
public void finishExecution(Path execRoot, SandboxOutputs outputs) {}
9898

9999
@Override
100100
boolean diedUnexpectedly() {

‎src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,7 @@ WorkResponse execInWorker(
483483
try {
484484
Stopwatch processOutputsStopwatch = Stopwatch.createStarted();
485485
context.lockOutputFiles();
486-
worker.finishExecution(execRoot);
486+
worker.finishExecution(execRoot, outputs);
487487
spawnMetrics.setProcessOutputsTime(processOutputsStopwatch.elapsed());
488488
} catch (IOException e) {
489489
restoreInterrupt(e);

‎src/test/java/com/google/devtools/build/lib/worker/WorkerExecRootTest.java‎

Lines changed: 54 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,17 @@ public void cleanFileSystem() throws Exception {
6262
Path workerSh = workspaceDir.getRelative("worker.sh");
6363
FileSystemUtils.writeContentAsLatin1(workerSh, "#!/bin/bash");
6464

65-
WorkerExecRoot workerExecRoot =
66-
new WorkerExecRoot(
67-
execRoot,
68-
new SandboxInputs(
69-
ImmutableMap.of(PathFragment.create("worker.sh"), workerSh),
70-
ImmutableSet.of(),
71-
ImmutableMap.of()),
72-
SandboxOutputs.create(
73-
ImmutableSet.of(PathFragment.create("very/output.txt")), ImmutableSet.of()),
74-
ImmutableSet.of(PathFragment.create("worker.sh")));
75-
workerExecRoot.createFileSystem();
65+
SandboxInputs inputs =
66+
new SandboxInputs(
67+
ImmutableMap.of(PathFragment.create("worker.sh"), workerSh),
68+
ImmutableSet.of(),
69+
ImmutableMap.of());
70+
SandboxOutputs outputs =
71+
SandboxOutputs.create(
72+
ImmutableSet.of(PathFragment.create("very/output.txt")), ImmutableSet.of());
73+
ImmutableSet<PathFragment> workerFiles = ImmutableSet.of(PathFragment.create("worker.sh"));
74+
WorkerExecRoot workerExecRoot = new WorkerExecRoot(execRoot);
75+
workerExecRoot.createFileSystem(workerFiles, inputs, outputs);
7676

7777
// Pretend to do some work inside the execRoot.
7878
execRoot.getRelative("tempdir").createDirectory();
@@ -82,7 +82,7 @@ public void cleanFileSystem() throws Exception {
8282
FileSystemUtils.writeContentAsLatin1(workerSh, "#!/bin/sh");
8383

8484
// Reuse the same execRoot.
85-
workerExecRoot.createFileSystem();
85+
workerExecRoot.createFileSystem(workerFiles, inputs, outputs);
8686

8787
assertThat(execRoot.getRelative("worker.sh").exists()).isTrue();
8888
assertThat(
@@ -102,21 +102,20 @@ public void createsAndCleansInputSymlinks() throws Exception {
102102
FileSystemUtils.ensureSymbolicLink(execRoot.getRelative("dir/input_symlink_2"), "unchanged");
103103
FileSystemUtils.ensureSymbolicLink(execRoot.getRelative("dir/input_symlink_3"), "whatever");
104104

105-
WorkerExecRoot workerExecRoot =
106-
new WorkerExecRoot(
107-
execRoot,
108-
new SandboxInputs(
109-
ImmutableMap.of(),
110-
ImmutableSet.of(),
111-
ImmutableMap.of(
112-
PathFragment.create("dir/input_symlink_1"), PathFragment.create("new_content"),
113-
PathFragment.create("dir/input_symlink_2"), PathFragment.create("unchanged"))),
114-
SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()),
115-
ImmutableSet.of());
105+
SandboxInputs inputs =
106+
new SandboxInputs(
107+
ImmutableMap.of(),
108+
ImmutableSet.of(),
109+
ImmutableMap.of(
110+
PathFragment.create("dir/input_symlink_1"), PathFragment.create("new_content"),
111+
PathFragment.create("dir/input_symlink_2"), PathFragment.create("unchanged")));
112+
SandboxOutputs outputs = SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of());
113+
ImmutableSet<PathFragment> workerFiles = ImmutableSet.of();
114+
WorkerExecRoot workerExecRoot = new WorkerExecRoot(execRoot);
116115

117116
// This should update the `input_symlink_{1,2,3}` according to `SandboxInputs`, i.e., update the
118117
// first/second (alternatively leave the second unchanged) and delete the third.
119-
workerExecRoot.createFileSystem();
118+
workerExecRoot.createFileSystem(workerFiles, inputs, outputs);
120119

121120
assertThat(execRoot.getRelative("dir/input_symlink_1").readSymbolicLink())
122121
.isEqualTo(PathFragment.create("new_content"));
@@ -127,23 +126,23 @@ public void createsAndCleansInputSymlinks() throws Exception {
127126

128127
@Test
129128
public void createsOutputDirs() throws Exception {
130-
WorkerExecRoot workerExecRoot =
131-
new WorkerExecRoot(
132-
execRoot,
133-
new SandboxInputs(ImmutableMap.of(), ImmutableSet.of(), ImmutableMap.of()),
134-
SandboxOutputs.create(
135-
ImmutableSet.of(
136-
PathFragment.create("dir/foo/bar_kt.jar"),
137-
PathFragment.create("dir/foo/bar_kt.jdeps"),
138-
PathFragment.create("dir/foo/bar_kt-sources.jar")),
139-
ImmutableSet.of(
140-
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_sourcegenfiles"),
141-
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_classes"),
142-
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_temp"),
143-
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_generated_classes"))),
144-
ImmutableSet.of());
145-
146-
workerExecRoot.createFileSystem();
129+
SandboxInputs inputs =
130+
new SandboxInputs(ImmutableMap.of(), ImmutableSet.of(), ImmutableMap.of());
131+
SandboxOutputs outputs =
132+
SandboxOutputs.create(
133+
ImmutableSet.of(
134+
PathFragment.create("dir/foo/bar_kt.jar"),
135+
PathFragment.create("dir/foo/bar_kt.jdeps"),
136+
PathFragment.create("dir/foo/bar_kt-sources.jar")),
137+
ImmutableSet.of(
138+
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_sourcegenfiles"),
139+
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_classes"),
140+
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_temp"),
141+
PathFragment.create("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_generated_classes")));
142+
ImmutableSet<PathFragment> workerFiles = ImmutableSet.of();
143+
WorkerExecRoot workerExecRoot = new WorkerExecRoot(execRoot);
144+
145+
workerExecRoot.createFileSystem(workerFiles, inputs, outputs);
147146

148147
assertThat(execRoot.getRelative("dir/foo/_kotlinc/bar_kt_jvm/bar_kt_sourcegenfiles").exists())
149148
.isTrue();
@@ -170,16 +169,15 @@ public void workspaceFilesAreNotDeleted() throws Exception {
170169
FileSystemUtils.ensureSymbolicLink(execRoot.getRelative("needed_file"), neededWorkspaceFile);
171170
FileSystemUtils.ensureSymbolicLink(execRoot.getRelative("other_file"), otherWorkspaceFile);
172171

173-
WorkerExecRoot workerExecRoot =
174-
new WorkerExecRoot(
175-
execRoot,
176-
new SandboxInputs(
177-
ImmutableMap.of(PathFragment.create("needed_file"), neededWorkspaceFile),
178-
ImmutableSet.of(),
179-
ImmutableMap.of()),
180-
SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()),
181-
ImmutableSet.of());
182-
workerExecRoot.createFileSystem();
172+
SandboxInputs inputs =
173+
new SandboxInputs(
174+
ImmutableMap.of(PathFragment.create("needed_file"), neededWorkspaceFile),
175+
ImmutableSet.of(),
176+
ImmutableMap.of());
177+
SandboxOutputs outputs = SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of());
178+
ImmutableSet<PathFragment> workerFiles = ImmutableSet.of();
179+
WorkerExecRoot workerExecRoot = new WorkerExecRoot(execRoot);
180+
workerExecRoot.createFileSystem(workerFiles, inputs, outputs);
183181

184182
assertThat(execRoot.getRelative("needed_file").readSymbolicLink())
185183
.isEqualTo(neededWorkspaceFile.asFragment());
@@ -199,17 +197,15 @@ public void recreatesEmptyFiles() throws Exception {
199197

200198
HashMap<PathFragment, Path> inputs = new HashMap<>();
201199
inputs.put(PathFragment.create("some_file"), null);
202-
WorkerExecRoot workerExecRoot =
203-
new WorkerExecRoot(
204-
execRoot,
205-
new SandboxInputs(inputs, ImmutableSet.of(), ImmutableMap.of()),
206-
SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()),
207-
ImmutableSet.of());
200+
SandboxInputs sandboxInputs = new SandboxInputs(inputs, ImmutableSet.of(), ImmutableMap.of());
201+
SandboxOutputs outputs = SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of());
202+
ImmutableSet<PathFragment> workerFiles = ImmutableSet.of();
203+
WorkerExecRoot workerExecRoot = new WorkerExecRoot(execRoot);
208204

209205
// This is interesting, because the filepath is a key in `SandboxInputs`, but its value is
210206
// `null`, which means "create an empty file". So after `createFileSystem` the file should be
211207
// empty.
212-
workerExecRoot.createFileSystem();
208+
workerExecRoot.createFileSystem(workerFiles, sandboxInputs, outputs);
213209

214210
assertThat(
215211
FileSystemUtils.readContent(

0 commit comments

Comments
 (0)