Make workers restart on flags that affect their creation/behaviour.

Also refactors the related code to being cleaner.

RELNOTES: None.
PiperOrigin-RevId: 374365649

Author: larsrc-google

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

@@ -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",

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

@@ -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;
@@ -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
@@ -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());
+    }
+  }
 }

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

@@ -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;
@@ -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();
@@ -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);
@@ -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(
@@ -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);
@@ -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(
@@ -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.",
@@ -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(
@@ -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);
+  }
 }