Skip to content

Commit

Permalink
Make workers restart on flags that affect their creation/behaviour.
Browse files Browse the repository at this point in the history 
Also refactors the related code to being cleaner.

RELNOTES: None.
PiperOrigin-RevId: 374365649
  • Loading branch information
@larsrc-google @copybara-github
larsrc-google authored and copybara-github committed May 18, 2021
1 parent c7d2616 commit 9dc95af
Show file tree
Hide file tree
Showing 13 changed files with 623 additions and 264 deletions.
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/worker/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
"//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
"//src/main/java/com/google/devtools/build/lib/exec/local",
"//src/main/java/com/google/devtools/build/lib/exec/local:options",
"//src/main/java/com/google/devtools/build/lib/runtime/commands/events",
"//src/main/java/com/google/devtools/build/lib/sandbox",
"//src/main/java/com/google/devtools/build/lib/shell",
Expand Down
97 changes: 95 additions & 2 deletions src/main/java/com/google/devtools/build/lib/worker/SimpleWorkerPool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.google.common.base.Throwables;
import java.io.IOException;
import java.util.Objects;
import javax.annotation.concurrent.ThreadSafe;
import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
Expand All @@ -28,8 +29,38 @@
@ThreadSafe
final class SimpleWorkerPool extends GenericKeyedObjectPool<WorkerKey, Worker> {

public SimpleWorkerPool(WorkerFactory factory, GenericKeyedObjectPoolConfig<Worker> config) {
super(factory, config);
public SimpleWorkerPool(WorkerFactory factory, int max) {
super(factory, makeConfig(max));
}

static SimpleWorkerPoolConfig makeConfig(int max) {
SimpleWorkerPoolConfig config = new SimpleWorkerPoolConfig();

// It's better to re-use a worker as often as possible and keep it hot, in order to profit
// from JIT optimizations as much as possible.
config.setLifo(true);

// Keep a fixed number of workers running per key.
config.setMaxIdlePerKey(max);
config.setMaxTotalPerKey(max);
config.setMinIdlePerKey(max);

// Don't limit the total number of worker processes, as otherwise the pool might be full of
// workers for one WorkerKey and can't accommodate a worker for another WorkerKey.
config.setMaxTotal(-1);

// Wait for a worker to become ready when a thread needs one.
config.setBlockWhenExhausted(true);

// Always test the liveliness of worker processes.
config.setTestOnBorrow(true);
config.setTestOnCreate(true);
config.setTestOnReturn(true);

// No eviction of idle workers.
config.setTimeBetweenEvictionRunsMillis(-1);

return config;
}

@Override
Expand All @@ -51,4 +82,66 @@ public void invalidateObject(WorkerKey key, Worker obj) throws IOException, Inte
throw new RuntimeException("unexpected", t);
}
}

/**
* Our own configuration class for the {@code SimpleWorkerPool} that correctly implements {@code
* equals()} and {@code hashCode()}.
*/
static final class SimpleWorkerPoolConfig extends GenericKeyedObjectPoolConfig<Worker> {
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
SimpleWorkerPoolConfig that = (SimpleWorkerPoolConfig) o;
return getBlockWhenExhausted() == that.getBlockWhenExhausted()
&& getFairness() == that.getFairness()
&& getJmxEnabled() == that.getJmxEnabled()
&& getLifo() == that.getLifo()
&& getMaxWaitMillis() == that.getMaxWaitMillis()
&& getMinEvictableIdleTimeMillis() == that.getMinEvictableIdleTimeMillis()
&& getNumTestsPerEvictionRun() == that.getNumTestsPerEvictionRun()
&& getSoftMinEvictableIdleTimeMillis() == that.getSoftMinEvictableIdleTimeMillis()
&& getTestOnBorrow() == that.getTestOnBorrow()
&& getTestOnCreate() == that.getTestOnCreate()
&& getTestOnReturn() == that.getTestOnReturn()
&& getTestWhileIdle() == that.getTestWhileIdle()
&& getTimeBetweenEvictionRunsMillis() == that.getTimeBetweenEvictionRunsMillis()
&& getMaxIdlePerKey() == that.getMaxIdlePerKey()
&& getMaxTotal() == that.getMaxTotal()
&& getMaxTotalPerKey() == that.getMaxTotalPerKey()
&& getMinIdlePerKey() == that.getMinIdlePerKey()
&& Objects.equals(getEvictionPolicyClassName(), that.getEvictionPolicyClassName())
&& Objects.equals(getJmxNameBase(), that.getJmxNameBase())
&& Objects.equals(getJmxNamePrefix(), that.getJmxNamePrefix());
}

@Override
public int hashCode() {
return Objects.hash(
getBlockWhenExhausted(),
getFairness(),
getJmxEnabled(),
getLifo(),
getMaxWaitMillis(),
getMinEvictableIdleTimeMillis(),
getNumTestsPerEvictionRun(),
getSoftMinEvictableIdleTimeMillis(),
getTestOnBorrow(),
getTestOnCreate(),
getTestOnReturn(),
getTestWhileIdle(),
getTimeBetweenEvictionRunsMillis(),
getMaxIdlePerKey(),
getMaxTotal(),
getMaxTotalPerKey(),
getMinIdlePerKey(),
getEvictionPolicyClassName(),
getJmxNameBase(),
getJmxNamePrefix());
}
}
}
42 changes: 27 additions & 15 deletions src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Objects;
import java.util.Optional;
import java.util.TreeSet;
import java.util.concurrent.atomic.AtomicInteger;
Expand All @@ -35,23 +36,19 @@ class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker> {
// request_id (which is indistinguishable from 0 in proto3).
private static final AtomicInteger pidCounter = new AtomicInteger(1);

private WorkerOptions workerOptions;
private final Path workerBaseDir;
private Reporter reporter;
private final boolean workerSandboxing;

public WorkerFactory(WorkerOptions workerOptions, Path workerBaseDir) {
this.workerOptions = workerOptions;
public WorkerFactory(Path workerBaseDir, boolean workerSandboxing) {
this.workerBaseDir = workerBaseDir;
this.workerSandboxing = workerSandboxing;
}

public void setReporter(Reporter reporter) {
this.reporter = reporter;
}

public void setOptions(WorkerOptions workerOptions) {
this.workerOptions = workerOptions;
}

@Override
public Worker create(WorkerKey key) {
int workerId = pidCounter.getAndIncrement();
Expand All @@ -60,7 +57,7 @@ public Worker create(WorkerKey key) {
workerBaseDir.getRelative(workTypeName + "-" + workerId + "-" + key.getMnemonic() + ".log");

Worker worker;
boolean sandboxed = workerOptions.workerSandboxing || key.isSpeculative();
boolean sandboxed = workerSandboxing || key.isSpeculative();
if (sandboxed) {
Path workDir = getSandboxedWorkerPath(key, workerId);
worker = new SandboxedWorker(key, workerId, workDir, logFile);
Expand All @@ -70,7 +67,7 @@ public Worker create(WorkerKey key) {
} else {
worker = new SingleplexWorker(key, workerId, key.getExecRoot(), logFile);
}
if (workerOptions.workerVerbose) {
if (reporter != null) {
reporter.handle(
Event.info(
String.format(
Expand All @@ -91,9 +88,7 @@ Path getSandboxedWorkerPath(WorkerKey key, int workerId) {
.getRelative(workspaceName);
}

/**
* Use the DefaultPooledObject implementation.
*/
/** Use the DefaultPooledObject implementation. */
@Override
public PooledObject<Worker> wrap(Worker worker) {
return new DefaultPooledObject<>(worker);
Expand All @@ -102,7 +97,7 @@ public PooledObject<Worker> wrap(Worker worker) {
/** When a worker process is discarded, destroy its process, too. */
@Override
public void destroyObject(WorkerKey key, PooledObject<Worker> p) {
if (workerOptions.workerVerbose) {
if (reporter != null) {
int workerId = p.getObject().getWorkerId();
reporter.handle(
Event.info(
Expand All @@ -122,7 +117,7 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
Worker worker = p.getObject();
Optional<Integer> exitValue = worker.getExitValue();
if (exitValue.isPresent()) {
if (workerOptions.workerVerbose && worker.diedUnexpectedly()) {
if (reporter != null && worker.diedUnexpectedly()) {
String msg =
String.format(
"%s %s (id %d) has unexpectedly died with exit code %d.",
Expand All @@ -140,7 +135,7 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
boolean filesChanged =
!key.getWorkerFilesCombinedHash().equals(worker.getWorkerFilesCombinedHash());

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

return !filesChanged;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof WorkerFactory)) {
return false;
}
WorkerFactory that = (WorkerFactory) o;
return workerSandboxing == that.workerSandboxing && workerBaseDir.equals(that.workerBaseDir);
}

@Override
public int hashCode() {
return Objects.hash(workerBaseDir, workerSandboxing);
}
}
Loading

0 comments on commit 9dc95af

Please sign in to comment.