Skip to content

Commit 9dc95af

Browse files
larsrc-googlecopybara-github
larsrc-google
authored and
copybara-github
committed
Make workers restart on flags that affect their creation/behaviour.
Also refactors the related code to being cleaner. RELNOTES: None. PiperOrigin-RevId: 374365649
1 parent c7d2616 commit 9dc95af

File tree

13 files changed

+623
-264
lines changed

13 files changed

+623
-264
lines changed

‎src/main/java/com/google/devtools/build/lib/worker/BUILD‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ java_library(
2626
"//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
2727
"//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
2828
"//src/main/java/com/google/devtools/build/lib/exec/local",
29-
"//src/main/java/com/google/devtools/build/lib/exec/local:options",
3029
"//src/main/java/com/google/devtools/build/lib/runtime/commands/events",
3130
"//src/main/java/com/google/devtools/build/lib/sandbox",
3231
"//src/main/java/com/google/devtools/build/lib/shell",

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

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import com.google.common.base.Throwables;
1717
import java.io.IOException;
18+
import java.util.Objects;
1819
import javax.annotation.concurrent.ThreadSafe;
1920
import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
2021
import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
@@ -28,8 +29,38 @@
2829
@ThreadSafe
2930
final class SimpleWorkerPool extends GenericKeyedObjectPool<WorkerKey, Worker> {
3031

31-
public SimpleWorkerPool(WorkerFactory factory, GenericKeyedObjectPoolConfig<Worker> config) {
32-
super(factory, config);
32+
public SimpleWorkerPool(WorkerFactory factory, int max) {
33+
super(factory, makeConfig(max));
34+
}
35+
36+
static SimpleWorkerPoolConfig makeConfig(int max) {
37+
SimpleWorkerPoolConfig config = new SimpleWorkerPoolConfig();
38+
39+
// It's better to re-use a worker as often as possible and keep it hot, in order to profit
40+
// from JIT optimizations as much as possible.
41+
config.setLifo(true);
42+
43+
// Keep a fixed number of workers running per key.
44+
config.setMaxIdlePerKey(max);
45+
config.setMaxTotalPerKey(max);
46+
config.setMinIdlePerKey(max);
47+
48+
// Don't limit the total number of worker processes, as otherwise the pool might be full of
49+
// workers for one WorkerKey and can't accommodate a worker for another WorkerKey.
50+
config.setMaxTotal(-1);
51+
52+
// Wait for a worker to become ready when a thread needs one.
53+
config.setBlockWhenExhausted(true);
54+
55+
// Always test the liveliness of worker processes.
56+
config.setTestOnBorrow(true);
57+
config.setTestOnCreate(true);
58+
config.setTestOnReturn(true);
59+
60+
// No eviction of idle workers.
61+
config.setTimeBetweenEvictionRunsMillis(-1);
62+
63+
return config;
3364
}
3465

3566
@Override
@@ -51,4 +82,66 @@ public void invalidateObject(WorkerKey key, Worker obj) throws IOException, Inte
5182
throw new RuntimeException("unexpected", t);
5283
}
5384
}
85+
86+
/**
87+
* Our own configuration class for the {@code SimpleWorkerPool} that correctly implements {@code
88+
* equals()} and {@code hashCode()}.
89+
*/
90+
static final class SimpleWorkerPoolConfig extends GenericKeyedObjectPoolConfig<Worker> {
91+
@Override
92+
public boolean equals(Object o) {
93+
if (this == o) {
94+
return true;
95+
}
96+
if (o == null || getClass() != o.getClass()) {
97+
return false;
98+
}
99+
SimpleWorkerPoolConfig that = (SimpleWorkerPoolConfig) o;
100+
return getBlockWhenExhausted() == that.getBlockWhenExhausted()
101+
&& getFairness() == that.getFairness()
102+
&& getJmxEnabled() == that.getJmxEnabled()
103+
&& getLifo() == that.getLifo()
104+
&& getMaxWaitMillis() == that.getMaxWaitMillis()
105+
&& getMinEvictableIdleTimeMillis() == that.getMinEvictableIdleTimeMillis()
106+
&& getNumTestsPerEvictionRun() == that.getNumTestsPerEvictionRun()
107+
&& getSoftMinEvictableIdleTimeMillis() == that.getSoftMinEvictableIdleTimeMillis()
108+
&& getTestOnBorrow() == that.getTestOnBorrow()
109+
&& getTestOnCreate() == that.getTestOnCreate()
110+
&& getTestOnReturn() == that.getTestOnReturn()
111+
&& getTestWhileIdle() == that.getTestWhileIdle()
112+
&& getTimeBetweenEvictionRunsMillis() == that.getTimeBetweenEvictionRunsMillis()
113+
&& getMaxIdlePerKey() == that.getMaxIdlePerKey()
114+
&& getMaxTotal() == that.getMaxTotal()
115+
&& getMaxTotalPerKey() == that.getMaxTotalPerKey()
116+
&& getMinIdlePerKey() == that.getMinIdlePerKey()
117+
&& Objects.equals(getEvictionPolicyClassName(), that.getEvictionPolicyClassName())
118+
&& Objects.equals(getJmxNameBase(), that.getJmxNameBase())
119+
&& Objects.equals(getJmxNamePrefix(), that.getJmxNamePrefix());
120+
}
121+
122+
@Override
123+
public int hashCode() {
124+
return Objects.hash(
125+
getBlockWhenExhausted(),
126+
getFairness(),
127+
getJmxEnabled(),
128+
getLifo(),
129+
getMaxWaitMillis(),
130+
getMinEvictableIdleTimeMillis(),
131+
getNumTestsPerEvictionRun(),
132+
getSoftMinEvictableIdleTimeMillis(),
133+
getTestOnBorrow(),
134+
getTestOnCreate(),
135+
getTestOnReturn(),
136+
getTestWhileIdle(),
137+
getTimeBetweenEvictionRunsMillis(),
138+
getMaxIdlePerKey(),
139+
getMaxTotal(),
140+
getMaxTotalPerKey(),
141+
getMinIdlePerKey(),
142+
getEvictionPolicyClassName(),
143+
getJmxNameBase(),
144+
getJmxNamePrefix());
145+
}
146+
}
54147
}

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

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.google.devtools.build.lib.events.Reporter;
1919
import com.google.devtools.build.lib.vfs.Path;
2020
import com.google.devtools.build.lib.vfs.PathFragment;
21+
import java.util.Objects;
2122
import java.util.Optional;
2223
import java.util.TreeSet;
2324
import java.util.concurrent.atomic.AtomicInteger;
@@ -35,23 +36,19 @@ class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker> {
3536
// request_id (which is indistinguishable from 0 in proto3).
3637
private static final AtomicInteger pidCounter = new AtomicInteger(1);
3738

38-
private WorkerOptions workerOptions;
3939
private final Path workerBaseDir;
4040
private Reporter reporter;
41+
private final boolean workerSandboxing;
4142

42-
public WorkerFactory(WorkerOptions workerOptions, Path workerBaseDir) {
43-
this.workerOptions = workerOptions;
43+
public WorkerFactory(Path workerBaseDir, boolean workerSandboxing) {
4444
this.workerBaseDir = workerBaseDir;
45+
this.workerSandboxing = workerSandboxing;
4546
}
4647

4748
public void setReporter(Reporter reporter) {
4849
this.reporter = reporter;
4950
}
5051

51-
public void setOptions(WorkerOptions workerOptions) {
52-
this.workerOptions = workerOptions;
53-
}
54-
5552
@Override
5653
public Worker create(WorkerKey key) {
5754
int workerId = pidCounter.getAndIncrement();
@@ -60,7 +57,7 @@ public Worker create(WorkerKey key) {
6057
workerBaseDir.getRelative(workTypeName + "-" + workerId + "-" + key.getMnemonic() + ".log");
6158

6259
Worker worker;
63-
boolean sandboxed = workerOptions.workerSandboxing || key.isSpeculative();
60+
boolean sandboxed = workerSandboxing || key.isSpeculative();
6461
if (sandboxed) {
6562
Path workDir = getSandboxedWorkerPath(key, workerId);
6663
worker = new SandboxedWorker(key, workerId, workDir, logFile);
@@ -70,7 +67,7 @@ public Worker create(WorkerKey key) {
7067
} else {
7168
worker = new SingleplexWorker(key, workerId, key.getExecRoot(), logFile);
7269
}
73-
if (workerOptions.workerVerbose) {
70+
if (reporter != null) {
7471
reporter.handle(
7572
Event.info(
7673
String.format(
@@ -91,9 +88,7 @@ Path getSandboxedWorkerPath(WorkerKey key, int workerId) {
9188
.getRelative(workspaceName);
9289
}
9390

94-
/**
95-
* Use the DefaultPooledObject implementation.
96-
*/
91+
/** Use the DefaultPooledObject implementation. */
9792
@Override
9893
public PooledObject<Worker> wrap(Worker worker) {
9994
return new DefaultPooledObject<>(worker);
@@ -102,7 +97,7 @@ public PooledObject<Worker> wrap(Worker worker) {
10297
/** When a worker process is discarded, destroy its process, too. */
10398
@Override
10499
public void destroyObject(WorkerKey key, PooledObject<Worker> p) {
105-
if (workerOptions.workerVerbose) {
100+
if (reporter != null) {
106101
int workerId = p.getObject().getWorkerId();
107102
reporter.handle(
108103
Event.info(
@@ -122,7 +117,7 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
122117
Worker worker = p.getObject();
123118
Optional<Integer> exitValue = worker.getExitValue();
124119
if (exitValue.isPresent()) {
125-
if (workerOptions.workerVerbose && worker.diedUnexpectedly()) {
120+
if (reporter != null && worker.diedUnexpectedly()) {
126121
String msg =
127122
String.format(
128123
"%s %s (id %d) has unexpectedly died with exit code %d.",
@@ -140,7 +135,7 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
140135
boolean filesChanged =
141136
!key.getWorkerFilesCombinedHash().equals(worker.getWorkerFilesCombinedHash());
142137

143-
if (workerOptions.workerVerbose && reporter != null && filesChanged) {
138+
if (reporter != null && filesChanged) {
144139
StringBuilder msg = new StringBuilder();
145140
msg.append(
146141
String.format(
@@ -167,4 +162,21 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
167162

168163
return !filesChanged;
169164
}
165+
166+
@Override
167+
public boolean equals(Object o) {
168+
if (this == o) {
169+
return true;
170+
}
171+
if (!(o instanceof WorkerFactory)) {
172+
return false;
173+
}
174+
WorkerFactory that = (WorkerFactory) o;
175+
return workerSandboxing == that.workerSandboxing && workerBaseDir.equals(that.workerBaseDir);
176+
}
177+
178+
@Override
179+
public int hashCode() {
180+
return Objects.hash(workerBaseDir, workerSandboxing);
181+
}
170182
}

0 commit comments

Comments
 (0)