From 8e359e7f1b707a45f61ebf96510988eae07f13b4 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 6 Apr 2023 05:08:21 -0700 Subject: [PATCH] RELNOTES[INC]: Remove high priority workers functionality from blaze. We don't observe any profit in wall time from high priority workers right now in Goole internally. Moreover for fully-worker builds we see a wall time degradation. PiperOrigin-RevId: 522309316 Change-Id: Ic5ebd3021d33abb043fca6f55edb0dc0468f4bef --- .../build/lib/actions/ResourceManager.java | 4 +- .../lib/bazel/rules/BazelRulesModule.java | 10 ++ .../build/lib/worker/WorkerModule.java | 5 +- .../build/lib/worker/WorkerOptions.java | 11 -- .../devtools/build/lib/worker/WorkerPool.java | 2 - .../build/lib/worker/WorkerPoolImpl.java | 90 +-------------- .../lib/actions/ResourceManagerTest.java | 98 ---------------- .../worker/WorkerLifecycleManagerTest.java | 31 ++--- .../build/lib/worker/WorkerModuleTest.java | 63 ----------- .../build/lib/worker/WorkerPoolTest.java | 107 +----------------- .../lib/worker/WorkerSpawnRunnerTest.java | 1 - 11 files changed, 31 insertions(+), 391 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java b/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java index 0350701e5d351a..8c87dd42e72b03 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java @@ -545,9 +545,7 @@ boolean areResourcesAvailable(ResourceSet resources) throws NoSuchElementExcepti availableWorkers = this.workerPool.getMaxTotalPerKey(workerKey); activeWorkers = this.workerPool.getNumActive(workerKey); } - boolean workerIsAvailable = - workerKey == null - || (activeWorkers < availableWorkers && workerPool.couldBeBorrowed(workerKey)); + boolean workerIsAvailable = workerKey == null || (activeWorkers < availableWorkers); // We test for tracking of extra resources whenever acquired and throw an // exception before acquiring any untracked resource. diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index ed48484188b5b6..54cf501f42c30b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -44,6 +44,7 @@ import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsBase; import java.io.IOException; +import java.util.List; /** Module implementing the rule set of Bazel. */ public final class BazelRulesModule extends BlazeModule { @@ -289,6 +290,15 @@ public static class BuildGraveyardOptions extends OptionsBase { effectTags = {OptionEffectTag.NO_OP}, help = "No-op.") public boolean parseHeadersSkippedIfCorrespondingSrcsFound; + + @Option( + name = "high_priority_workers", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = "No-op, will be removed soon.", + allowMultiple = true) + public List highPriorityWorkers; } /** This is where deprecated Bazel-specific options only used by the build command go to die. */ diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 49367d71ae48d8..410962f10182d4 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java @@ -145,10 +145,7 @@ public void buildStarting(BuildStartingEvent event) { WorkerPoolConfig newConfig = new WorkerPoolConfig( - workerFactory, - options.workerMaxInstances, - options.workerMaxMultiplexInstances, - options.highPriorityWorkers); + workerFactory, options.workerMaxInstances, options.workerMaxMultiplexInstances); // If the config changed compared to the last run, we have to create a new pool. if (workerPool == null || !newConfig.equals(workerPool.getWorkerPoolConfig())) { diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java index 2e20438cd7c84e..93942abb40bc46 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java @@ -108,17 +108,6 @@ public String getTypeDescription() { allowMultiple = true) public List> workerMaxMultiplexInstances; - @Option( - name = "high_priority_workers", - defaultValue = "null", - documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, - effectTags = {OptionEffectTag.EXECUTION}, - help = - "Mnemonics of workers to run with high priority. When high priority workers are running " - + "all other workers are throttled.", - allowMultiple = true) - public List highPriorityWorkers; - @Option( name = "worker_quit_after_build", defaultValue = "false", diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java index 599bab05d2d1b9..d7ff522f2ab549 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java @@ -38,8 +38,6 @@ public interface WorkerPool { Worker borrowObject(WorkerKey key) throws IOException, InterruptedException; - boolean couldBeBorrowed(WorkerKey key); - void returnObject(WorkerKey key, Worker obj); void invalidateObject(WorkerKey key, Worker obj) throws InterruptedException; diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolImpl.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolImpl.java index e47efd0f9383e2..7324db2f71158b 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolImpl.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolImpl.java @@ -23,7 +23,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Objects; -import java.util.concurrent.atomic.AtomicInteger; import javax.annotation.Nonnull; import javax.annotation.concurrent.ThreadSafe; import org.apache.commons.pool2.impl.EvictionPolicy; @@ -37,15 +36,6 @@ public class WorkerPoolImpl implements WorkerPool { /** Unless otherwise specified, the max number of multiplex workers per WorkerKey. */ private static final int DEFAULT_MAX_MULTIPLEX_WORKERS = 8; - private static final int MAX_NON_BLOCKING_HIGH_PRIORITY_WORKERS = 1; - /** - * How many high-priority workers are currently borrowed. If greater than one, low-priority - * workers cannot be borrowed until the high-priority ones are done. - */ - private final AtomicInteger highPriorityWorkersInUse = new AtomicInteger(0); - /** Which mnemonics create high-priority workers. */ - private final ImmutableSet highPriorityWorkerMnemonics; - private final WorkerPoolConfig workerPoolConfig; /** Map of singleplex worker pools, one per mnemonic. */ private final ImmutableMap workerPools; @@ -58,9 +48,6 @@ public class WorkerPoolImpl implements WorkerPool { public WorkerPoolImpl(WorkerPoolConfig workerPoolConfig) { this.workerPoolConfig = workerPoolConfig; - highPriorityWorkerMnemonics = - ImmutableSet.copyOf((Iterable) workerPoolConfig.getHighPriorityWorkers()); - ImmutableMap config = createConfigFromOptions(workerPoolConfig.getWorkerMaxInstances(), DEFAULT_MAX_WORKERS); ImmutableMap multiplexConfig = @@ -143,8 +130,7 @@ public void evictWithPolicy(EvictionPolicy evictionPolicy) throws Interr } /** - * Gets a worker. May block indefinitely if too many high-priority workers are in use and the - * requested worker is not high priority. + * Gets a worker from worker pool. Could wait if no idle workers are available. * * @param key worker key * @return a worker @@ -158,44 +144,11 @@ public Worker borrowObject(WorkerKey key) throws IOException, InterruptedExcepti Throwables.propagateIfPossible(t, IOException.class, InterruptedException.class); throw new RuntimeException("unexpected", t); } - - // TODO(b/244297036): move highPriorityWorkerMnemonics logic to the ResourceManager. - if (highPriorityWorkerMnemonics.contains(key.getMnemonic())) { - highPriorityWorkersInUse.incrementAndGet(); - } else { - try { - waitForHighPriorityWorkersToFinish(); - } catch (InterruptedException e) { - returnObject(key, result); - throw e; - } - } - return result; } - /** - * Checks if there is no blockers from high priority workers to take new worker with this worker - * key. Doesn't check occupancy of worker pool for this mnemonic. - */ - @Override - public boolean couldBeBorrowed(WorkerKey key) { - if (highPriorityWorkerMnemonics.contains(key.getMnemonic())) { - return true; - } - - if (highPriorityWorkersInUse.get() <= MAX_NON_BLOCKING_HIGH_PRIORITY_WORKERS) { - return true; - } - - return false; - } - @Override public void returnObject(WorkerKey key, Worker obj) { - if (highPriorityWorkerMnemonics.contains(key.getMnemonic())) { - decrementHighPriorityWorkerCount(); - } if (doomedWorkers.contains(obj.getWorkerId())) { obj.setDoomed(true); } @@ -204,9 +157,6 @@ public void returnObject(WorkerKey key, Worker obj) { @Override public void invalidateObject(WorkerKey key, Worker obj) throws InterruptedException { - if (highPriorityWorkerMnemonics.contains(key.getMnemonic())) { - decrementHighPriorityWorkerCount(); - } if (doomedWorkers.contains(obj.getWorkerId())) { obj.setDoomed(true); } @@ -218,29 +168,6 @@ public void invalidateObject(WorkerKey key, Worker obj) throws InterruptedExcept } } - // Decrements the high-priority workers counts and pings waiting threads if appropriate. - private void decrementHighPriorityWorkerCount() { - if (highPriorityWorkersInUse.decrementAndGet() <= MAX_NON_BLOCKING_HIGH_PRIORITY_WORKERS) { - synchronized (highPriorityWorkersInUse) { - highPriorityWorkersInUse.notifyAll(); - } - } - } - - // Returns once less than two high-priority workers are running. - private void waitForHighPriorityWorkersToFinish() throws InterruptedException { - // Fast path for the case where the high-priority workers feature is not in use. - if (highPriorityWorkerMnemonics.isEmpty()) { - return; - } - - while (highPriorityWorkersInUse.get() > MAX_NON_BLOCKING_HIGH_PRIORITY_WORKERS) { - synchronized (highPriorityWorkersInUse) { - highPriorityWorkersInUse.wait(); - } - } - } - @Override public synchronized void setDoomedWorkers(ImmutableSet workerIds) { this.doomedWorkers = workerIds; @@ -280,17 +207,14 @@ public static class WorkerPoolConfig { private final WorkerFactory workerFactory; private final List> workerMaxInstances; private final List> workerMaxMultiplexInstances; - private final List highPriorityWorkers; public WorkerPoolConfig( WorkerFactory workerFactory, List> workerMaxInstances, - List> workerMaxMultiplexInstances, - List highPriorityWorkers) { + List> workerMaxMultiplexInstances) { this.workerFactory = workerFactory; this.workerMaxInstances = workerMaxInstances; this.workerMaxMultiplexInstances = workerMaxMultiplexInstances; - this.highPriorityWorkers = highPriorityWorkers; } public WorkerFactory getWorkerFactory() { @@ -305,10 +229,6 @@ public List> getWorkerMaxMultiplexInstances() { return workerMaxMultiplexInstances; } - public List getHighPriorityWorkers() { - return highPriorityWorkers; - } - @Override public boolean equals(Object o) { if (this == o) { @@ -320,14 +240,12 @@ public boolean equals(Object o) { WorkerPoolConfig that = (WorkerPoolConfig) o; return workerFactory.equals(that.workerFactory) && workerMaxInstances.equals(that.workerMaxInstances) - && workerMaxMultiplexInstances.equals(that.workerMaxMultiplexInstances) - && highPriorityWorkers.equals(that.highPriorityWorkers); + && workerMaxMultiplexInstances.equals(that.workerMaxMultiplexInstances); } @Override public int hashCode() { - return Objects.hash( - workerFactory, workerMaxInstances, workerMaxMultiplexInstances, highPriorityWorkers); + return Objects.hash(workerFactory, workerMaxInstances, workerMaxMultiplexInstances); } } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java index 016dae1f3310eb..32eb0b341a4068 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java @@ -41,10 +41,8 @@ import com.google.devtools.build.lib.worker.WorkerPoolImpl; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.IOException; -import java.time.Duration; import java.util.Collection; import java.util.NoSuchElementException; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -101,7 +99,6 @@ public boolean validateObject(WorkerKey key, PooledObject p) { } }, ImmutableList.of(), - ImmutableList.of(), ImmutableList.of())); } @@ -759,101 +756,6 @@ public void testAcquireWithWorker_acquireAndRelease() throws Exception { assertThat(rm.inUse()).isFalse(); } - @Test - public void testReleaseWorker_highPriorityWorker() throws Exception { - - String slowMenmonic = "SLOW"; - String fastMenmonic = "FAST"; - - Worker slowWorker1 = mock(Worker.class); - Worker slowWorker2 = mock(Worker.class); - Worker fastWorker = mock(Worker.class); - - WorkerKey slowWorkerKey = createWorkerKey(slowMenmonic); - WorkerKey fastWorkerKey = createWorkerKey(fastMenmonic); - - when(slowWorker1.getWorkerKey()).thenReturn(slowWorkerKey); - when(slowWorker2.getWorkerKey()).thenReturn(slowWorkerKey); - when(fastWorker.getWorkerKey()).thenReturn(fastWorkerKey); - - CountDownLatch slowLatch = new CountDownLatch(2); - CountDownLatch fastLatch = new CountDownLatch(1); - - WorkerPoolImpl workerPool = - new WorkerPoolImpl( - new WorkerPoolImpl.WorkerPoolConfig( - new WorkerFactory(fs.getPath("/workerBase")) { - int numOfSlowWorkers = 0; - - @Override - public Worker create(WorkerKey key) { - assertThat(key.getMnemonic()).isAnyOf(slowMenmonic, fastMenmonic); - - if (key.getMnemonic().equals(fastMenmonic)) { - return fastWorker; - } - - assertThat(numOfSlowWorkers).isLessThan(2); - - if (numOfSlowWorkers == 0) { - numOfSlowWorkers++; - return slowWorker1; - } - - numOfSlowWorkers++; - return slowWorker2; - } - - @Override - public boolean validateObject(WorkerKey key, PooledObject p) { - return true; - } - }, - ImmutableList.of(), - ImmutableList.of(), - /* highPriorityWorkers= */ ImmutableList.of(slowMenmonic))); - rm.setWorkerPool(workerPool); - - TestThread slowThread1 = - new TestThread( - () -> { - ResourceHandle handle = acquire(100, 0.1, 0, slowMenmonic); - slowLatch.countDown(); - fastLatch.await(); - // release resources - handle.close(); - }); - - TestThread slowThread2 = - new TestThread( - () -> { - ResourceHandle handle = acquire(100, 0.1, 0, slowMenmonic); - slowLatch.countDown(); - fastLatch.await(); - // release resources - handle.close(); - }); - - TestThread fastThread = - new TestThread( - () -> { - slowLatch.await(); - assertThat(isAvailable(rm, 100, 0.1, 0, createWorkerKey(fastMenmonic))).isFalse(); - fastLatch.countDown(); - ResourceHandle handle = acquire(100, 0.1, 0, fastMenmonic); - // release resources - handle.close(); - }); - - slowThread1.start(); - slowThread2.start(); - fastThread.start(); - - slowThread1.joinAndAssertState(Duration.ofSeconds(10).toMillis()); - slowThread2.joinAndAssertState(Duration.ofSeconds(10).toMillis()); - fastThread.joinAndAssertState(Duration.ofSeconds(10).toMillis()); - } - synchronized boolean isAvailable(ResourceManager rm, double ram, double cpu, int localTestCount) { return rm.areResourcesAvailable(ResourceSet.create(ram, cpu, localTestCount)); } diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerLifecycleManagerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerLifecycleManagerTest.java index fc4a10200fbd65..c0f3f7ee7470f8 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerLifecycleManagerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerLifecycleManagerTest.java @@ -20,7 +20,6 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.devtools.build.lib.clock.BlazeClock; @@ -69,8 +68,7 @@ public void setUp() throws Exception { public void testEvictWorkers_doNothing_lowMemoryUsage() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 1), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 1), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); workerPool.returnObject(key, w1); @@ -99,8 +97,7 @@ public void testEvictWorkers_doNothing_lowMemoryUsage() throws Exception { public void testEvictWorkers_doNothing_zeroThreshold() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 1), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 1), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); workerPool.returnObject(key, w1); @@ -129,8 +126,7 @@ public void testEvictWorkers_doNothing_zeroThreshold() throws Exception { public void testEvictWorkers_doNothing_emptyMetrics() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 1), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 1), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); workerPool.returnObject(key, w1); @@ -154,8 +150,7 @@ public void testEvictWorkers_doNothing_emptyMetrics() throws Exception { public void testGetEvictionCandidates_selectOnlyWorker() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 1), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 1), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); workerPool.returnObject(key, w1); @@ -184,8 +179,7 @@ public void testGetEvictionCandidates_selectOnlyWorker() throws Exception { public void testGetEvictionCandidates_evictLargestWorkers() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 3), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 3), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); Worker w2 = workerPool.borrowObject(key); @@ -227,8 +221,7 @@ public void testGetEvictionCandidates_evictLargestWorkers() throws Exception { public void testGetEvictionCandidates_evictOnlyIdleWorkers() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 3), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 3), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); Worker w2 = workerPool.borrowObject(key); @@ -269,11 +262,7 @@ public void testGetEvictionCandidates_evictOnlyIdleWorkers() throws Exception { public void testGetEvictionCandidates_evictDifferentWorkerKeys() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, - entryList("dummy", 2, "smart", 2), - emptyEntryList(), - Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 2, "smart", 2), emptyEntryList())); WorkerKey key1 = createWorkerKey("dummy", fileSystem); WorkerKey key2 = createWorkerKey("smart", fileSystem); Worker w1 = workerPool.borrowObject(key1); @@ -329,8 +318,7 @@ public void testGetEvictionCandidates_evictDifferentWorkerKeys() throws Exceptio public void testGetEvictionCandidates_testDoomedWorkers() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 2), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 2), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); Worker w2 = workerPool.borrowObject(key); @@ -367,8 +355,7 @@ public void testGetEvictionCandidates_testDoomedWorkers() throws Exception { public void testGetEvictionCandidates_testDoomedAndIdleWorkers() throws Exception { WorkerPoolImpl workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("dummy", 5), emptyEntryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("dummy", 5), emptyEntryList())); WorkerKey key = createWorkerKey("dummy", fileSystem); Worker w1 = workerPool.borrowObject(key); Worker w2 = workerPool.borrowObject(key); diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java index 9c255517fc54fb..9c3ac4f6d0aafc 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java @@ -41,7 +41,6 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; -import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.OptionsParsingResult; import java.io.IOException; import org.junit.Rule; @@ -110,40 +109,6 @@ public void buildStarting_noRestartOnSandboxChange() throws IOException, AbruptE assertThat(aLog.exists()).isTrue(); } - @Test - public void buildStarting_workersDestroyedOnRestart() - throws IOException, AbruptExitException, InterruptedException, OptionsParsingException { - WorkerModule module = new WorkerModule(); - WorkerOptions options = WorkerOptions.DEFAULTS; - options.workerVerbose = true; - when(request.getOptions(WorkerOptions.class)).thenReturn(options); - setupEnvironment("/outputRoot"); - - module.beforeCommand(env); - module.buildStarting(BuildStartingEvent.create(env, request)); - WorkerKey workerKey = TestUtils.createWorkerKey(JSON, fs, true); - Worker worker = module.workerPool.borrowObject(workerKey); - assertThat(worker.workerKey).isEqualTo(workerKey); - assertThat(storedEventHandler.getEvents()).hasSize(1); - assertThat(storedEventHandler.getEvents().get(0).getMessage()) - .contains("Created new sandboxed dummy worker"); - storedEventHandler.clear(); - - Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers"); - Path aLog = workerDir.getRelative("f.log"); - workerDir.createDirectoryAndParents(); - aLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT); - WorkerPool oldPool = module.workerPool; - options.highPriorityWorkers = ImmutableList.of("Foobar"); - module.beforeCommand(env); - module.buildStarting(BuildStartingEvent.create(env, request)); - assertThat(storedEventHandler.getEvents()).hasSize(1); - assertThat(storedEventHandler.getEvents().get(0).getMessage()) - .contains("Worker pool configuration has changed"); - assertThat(module.workerPool).isNotSameInstanceAs(oldPool); - assertThat(aLog.exists()).isTrue(); - } - @Test public void buildStarting_restartsOnOutputbaseChanges() throws IOException, AbruptExitException { WorkerModule module = new WorkerModule(); @@ -195,34 +160,6 @@ public void buildStarting_clearsLogsOnFactoryCreation() throws IOException, Abru assertThat(oldLog.exists()).isFalse(); } - @Test - public void buildStarting_restartsOnHiPrioChanges() throws IOException, AbruptExitException { - WorkerModule module = new WorkerModule(); - WorkerOptions options = WorkerOptions.DEFAULTS; - when(request.getOptions(WorkerOptions.class)).thenReturn(options); - setupEnvironment("/outputRoot"); - - module.beforeCommand(env); - // Check that new pools/factories are made with default options - module.buildStarting(BuildStartingEvent.create(env, request)); - assertThat(storedEventHandler.getEvents()).isEmpty(); - - // Logs are only cleared on factory reset, not on pool reset, so this file should survive - Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers"); - Path oldLog = workerDir.getRelative("f.log"); - workerDir.createDirectoryAndParents(); - oldLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT); - - WorkerPool oldPool = module.workerPool; - options.highPriorityWorkers = ImmutableList.of("foo"); - module.beforeCommand(env); - module.buildStarting(BuildStartingEvent.create(env, request)); - assertThat(storedEventHandler.getEvents()).hasSize(1); - assertThat(storedEventHandler.getEvents().get(0).getMessage()) - .contains("Worker pool configuration has changed"); - assertThat(module.workerPool).isNotSameInstanceAs(oldPool); - assertThat(oldLog.exists()).isTrue(); - } @Test public void buildStarting_restartsOnNumMultiplexWorkersChanges() diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java index 6a36453e9c6e95..f2c0ee7f24d4f2 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.worker; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; import static com.google.devtools.build.lib.worker.TestUtils.createWorkerKey; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -24,7 +23,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -32,8 +30,6 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.build.lib.worker.WorkerPoolImpl.WorkerPoolConfig; -import java.io.IOException; -import java.lang.Thread.State; import java.util.Map.Entry; import org.apache.commons.pool2.impl.DefaultPooledObject; import org.junit.Before; @@ -79,8 +75,7 @@ public void setUp() throws Exception { public void testBorrow_createsWhenNeeded() throws Exception { WorkerPool workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("mnem", 2, "", 1), entryList())); WorkerKey workerKey = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey); Worker worker2 = workerPool.borrowObject(workerKey); @@ -93,8 +88,7 @@ public void testBorrow_createsWhenNeeded() throws Exception { public void testBorrow_reusesWhenPossible() throws Exception { WorkerPool workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("mnem", 2, "", 1), entryList())); WorkerKey workerKey = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey); workerPool.returnObject(workerKey, worker1); @@ -107,8 +101,7 @@ public void testBorrow_reusesWhenPossible() throws Exception { public void testBorrow_usesDefault() throws Exception { WorkerPool workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("mnem", 2, "", 1), entryList())); WorkerKey workerKey1 = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey1); Worker worker1a = workerPool.borrowObject(workerKey1); @@ -125,8 +118,7 @@ public void testBorrow_usesDefault() throws Exception { public void testBorrow_pooledByKey() throws Exception { WorkerPool workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("mnem", 2, "", 1), entryList())); WorkerKey workerKey1 = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey1); Worker worker1a = workerPool.borrowObject(workerKey1); @@ -144,10 +136,7 @@ public void testBorrow_separateMultiplexWorkers() throws Exception { WorkerPool workerPool = new WorkerPoolImpl( new WorkerPoolConfig( - factoryMock, - entryList("mnem", 1, "", 1), - entryList("mnem", 2, "", 1), - Lists.newArrayList())); + factoryMock, entryList("mnem", 1, "", 1), entryList("mnem", 2, "", 1))); WorkerKey workerKey = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey); assertThat(worker1.getWorkerId()).isEqualTo(1); @@ -166,88 +155,11 @@ public void testBorrow_separateMultiplexWorkers() throws Exception { verify(factoryMock, times(2)).makeObject(multiplexKey); } - @Test - public void testBorrow_allowsOneHiPrio() throws Exception { - WorkerPool workerPool = - new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, - entryList("loprio", 2, "hiprio", 2, "", 1), - entryList(), - ImmutableList.of("hiprio"))); - WorkerKey workerKey1 = createWorkerKey(fileSystem, "hiprio", false); - Worker worker1 = workerPool.borrowObject(workerKey1); - assertThat(worker1.getWorkerId()).isEqualTo(1); - // A single hiprio worker should not block. - WorkerKey workerKey2 = createWorkerKey(fileSystem, "loprio", false); - Worker worker2 = workerPool.borrowObject(workerKey2); - assertThat(worker2.getWorkerId()).isEqualTo(2); - verify(factoryMock, times(1)).makeObject(workerKey1); - verify(factoryMock, times(1)).makeObject(workerKey2); - } - - @Test - public void testBorrow_twoHiPrioBlocks() throws Exception { - WorkerPool workerPool = - new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, - entryList("loprio", 2, "hiprio", 2, "", 1), - entryList(), - ImmutableList.of("hiprio"))); - WorkerKey workerKey1 = createWorkerKey(fileSystem, "hiprio", false); - Worker worker1 = workerPool.borrowObject(workerKey1); - Worker worker1a = workerPool.borrowObject(workerKey1); - assertThat(worker1.getWorkerId()).isEqualTo(1); - assertThat(worker1a.getWorkerId()).isEqualTo(2); - WorkerKey workerKey2 = createWorkerKey(fileSystem, "loprio", false); - assertWithMessage("Could not borrow low priority worker") - .that(workerPool.couldBeBorrowed(workerKey2)) - .isFalse(); - Thread t = - new Thread( - () -> { - try { - workerPool.borrowObject(workerKey2); - } catch (IOException | InterruptedException e) { - // Ignorable - } - }); - t.start(); - boolean waited = false; - for (int tries = 0; tries < 1000; tries++) { - if (t.getState() == State.WAITING) { - waited = true; - break; - } - Thread.sleep(1); - } - assertWithMessage("Expected low-priority worker to wait").that(waited).isTrue(); - workerPool.returnObject(workerKey1, worker1); - assertWithMessage("Could not borrow low priority worker") - .that(workerPool.couldBeBorrowed(workerKey2)) - .isTrue(); - boolean continued = false; - for (int tries = 0; tries < 1000; tries++) { - if (t.getState() != State.WAITING) { - continued = true; - break; - } - Thread.sleep(1); - } - assertWithMessage("Expected low-priority worker to eventually continue") - .that(continued) - .isTrue(); - verify(factoryMock, times(2)).makeObject(workerKey1); - verify(factoryMock, times(1)).makeObject(workerKey2); - } - @Test public void testBorrow_doomedWorkers() throws Exception { WorkerPool workerPool = new WorkerPoolImpl( - new WorkerPoolConfig( - factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); + new WorkerPoolConfig(factoryMock, entryList("mnem", 2, "", 1), entryList())); WorkerKey workerKey = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey); Worker worker2 = workerPool.borrowObject(workerKey); @@ -272,11 +184,4 @@ private static ImmutableList> entryList( return ImmutableList.of(Maps.immutableEntry(key1, value1), Maps.immutableEntry(key2, value2)); } - private static ImmutableList> entryList( - String key1, int value1, String key2, int value2, String key3, int value3) { - return ImmutableList.of( - Maps.immutableEntry(key1, value1), - Maps.immutableEntry(key2, value2), - Maps.immutableEntry(key3, value3)); - } } diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index 3fd8758fca1a62..5d5d14ef31db7b 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -123,7 +123,6 @@ public boolean validateObject(WorkerKey key, PooledObject p) { } }, ImmutableList.of(), - ImmutableList.of(), ImmutableList.of())); }