Remove all uses of I/O in Resource sets
While these purported to model I/O usage, they only created an
artificial bottleneck that prevented more than 1/x (rounded down)
actions from running in parallel, regardless of how powerful the
underlying machine is since the available I/O was always modeled as 1.0.
At the same time, most actions were modeled as zero I/O, so there was no
constraint on the total number of actions running. The constraints
mostly applied to actions of the same type.
In theory, overcommitting a machine on memory is the most problematic,
as this can cause arbitrary process kills and extreme slowdowns (due to
excessive swapping).
Overcommitting a machine on CPU is the next biggest problem; while Linux
handles CPU overcommit very gracefully, we know that MacOS does a much
worse job. However, we recently increased the minimum allocation to one
core per process, which makes it less likely to happen, and we allow
users to override specific instances with the 'cpu:n' tag.
Overcommitting on I/O primarily slows down execution of concurrent
processes. On a spinning platter, this can be particularly bad due to
seek thrashing, where multiple processes try to read files sequentially,
but due to interleaving, the disk ends up spending most of its time in
seeks, and this can dominate performance. With the increasing usage of
SSDs, which don't have any seek time, this becomes much less of a
concern. However, Bazel isn't in a good position to control this, and
the current approach is basically just broken. A good disk scheduler can
still handle multiple processes without too much of a slowdown, whereas
Bazel would have to make conservative restrictions about which processes
it runs in parallel. In addition, Bazel cannot even detect whether it's
running on an SSD, HDD, network file system, or ramdisk, so there's no
way to automatically detect this.
Overall, it seems better to rely on the existing mechanisms to prevent
memory and CPU overcommit to also prevent I/O overcommit. Individual
users can tweak the existing settings (e.g., by reducing jobs, by
setting --local_resources, or by increasing cpu settings on a per-rule
or per-action basis), if they are even affected.
There is reason to expect this change to be a net win in performance and
predictability for a majority of Bazel users, even if it's worse for a
small fraction.
Tests:
These settings were preventing more than 10 medium or large tests from
running at the same time. For tests that are actually in-memory unit
tests, this unnecessarily reduces parallelism on machines with 10+
cores. On machines with fewer cores, we're already constraining the
number of tests with jobs (by default jobs = cores), or, as of the
recent changes to CPU setting, with the cores counter. The additional
specification of 0.1 I/O seems unnecessary and more likely to do harm
than good. For enormous tests, it seems advisable to use the 'cpu:n' tag
to give Bazel a hint about the nature of the test.
Unfortunately, test size is not a well-defined concept, so it's unclear
whether we should increase the default core resource count for larger
tests; there are arguments both in favor and against such a move.
Ideally, we'd come up with a crisper definition, which would allow us to
make a better call here.
CppLinkAction:
These settings were preventing more than 3 link actions from running at
the same time, regardless of how heavyweight those actions are. This
seems rather unfortunate - while we know that some link actions are so
large that they monopolize the machine, this should be modeled with
memory instead. While link actions may be the most likely candidate for
inducing seek thrashing, they are also the most predictable for a disk
scheduler to handle.
PiperOrigin-RevId: 218869304
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 233efc0..d294afd1 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
@@ -48,25 +48,24 @@
public final void configureResourceManager() throws Exception {
rm.setRamUtilizationPercentage(100);
rm.setAvailableResources(
- ResourceSet.create(
- /*memoryMb=*/ 1000.0, /*cpuUsage=*/ 1.0, /*ioUsage=*/ 1.0, /* localTestCount= */ 2));
+ ResourceSet.create(/*memoryMb=*/ 1000, /*cpuUsage=*/ 1, /* localTestCount= */ 2));
counter = new AtomicInteger(0);
sync = new CyclicBarrier(2);
sync2 = new CyclicBarrier(2);
rm.resetResourceUsage();
}
- private ResourceHandle acquire(double ram, double cpu, double io, int tests)
+ private ResourceHandle acquire(double ram, double cpu, int tests)
throws InterruptedException {
- return rm.acquireResources(resourceOwner, ResourceSet.create(ram, cpu, io, tests));
+ return rm.acquireResources(resourceOwner, ResourceSet.create(ram, cpu, tests));
}
- private ResourceHandle acquireNonblocking(double ram, double cpu, double io, int tests) {
- return rm.tryAcquire(resourceOwner, ResourceSet.create(ram, cpu, io, tests));
+ private ResourceHandle acquireNonblocking(double ram, double cpu, int tests) {
+ return rm.tryAcquire(resourceOwner, ResourceSet.create(ram, cpu, tests));
}
- private void release(double ram, double cpu, double io, int tests) {
- rm.releaseResources(resourceOwner, ResourceSet.create(ram, cpu, io, tests));
+ private void release(double ram, double cpu, int tests) {
+ rm.releaseResources(resourceOwner, ResourceSet.create(ram, cpu, tests));
}
private void validate(int count) {
@@ -80,34 +79,27 @@
// When nothing is consuming RAM,
// Then Resource Manager will successfully acquire an over-budget request for RAM:
double bigRam = 10000.0;
- acquire(bigRam, 0, 0, 0);
+ acquire(bigRam, 0, 0);
// When RAM is consumed,
// Then Resource Manager will be "in use":
assertThat(rm.inUse()).isTrue();
- release(bigRam, 0, 0, 0);
+ release(bigRam, 0, 0);
// When that RAM is released,
// Then Resource Manager will not be "in use":
assertThat(rm.inUse()).isFalse();
// Ditto, for CPU:
double bigCpu = 10.0;
- acquire(0, bigCpu, 0, 0);
+ acquire(0, bigCpu, 0);
assertThat(rm.inUse()).isTrue();
- release(0, bigCpu, 0, 0);
- assertThat(rm.inUse()).isFalse();
-
- // Ditto, for IO:
- double bigIo = 10.0;
- acquire(0, 0, bigIo, 0);
- assertThat(rm.inUse()).isTrue();
- release(0, 0, bigIo, 0);
+ release(0, bigCpu, 0);
assertThat(rm.inUse()).isFalse();
// Ditto, for tests:
int bigTests = 10;
- acquire(0, 0, 0, bigTests);
+ acquire(0, 0, bigTests);
assertThat(rm.inUse()).isTrue();
- release(0, 0, 0, bigTests);
+ release(0, 0, bigTests);
assertThat(rm.inUse()).isFalse();
}
@@ -116,7 +108,7 @@
assertThat(rm.inUse()).isFalse();
// Given CPU is partially acquired:
- acquire(0, 0.5, 0, 0);
+ acquire(0, 0.5, 0);
// When a request for CPU is made that would slightly overallocate CPU,
// Then the request succeeds:
@@ -124,7 +116,7 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- assertThat(acquireNonblocking(0, 0.6, 0, 0)).isNotNull();
+ assertThat(acquireNonblocking(0, 0.6, 0)).isNotNull();
}
};
thread1.start();
@@ -136,7 +128,7 @@
assertThat(rm.inUse()).isFalse();
// Given that CPU has a small initial allocation:
- acquire(0, 0.099, 0, 0);
+ acquire(0, 0.099, 0);
// When a request for a large CPU allocation is made,
// Then the request succeeds:
@@ -144,20 +136,20 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- assertThat(acquireNonblocking(0, 0.99, 0, 0)).isNotNull();
+ assertThat(acquireNonblocking(0, 0.99, 0)).isNotNull();
// Cleanup
- release(0, 0.99, 0, 0);
+ release(0, 0.99, 0);
}
};
thread1.start();
thread1.joinAndAssertState(10000);
// Cleanup
- release(0, 0.099, 0, 0);
+ release(0, 0.099, 0);
assertThat(rm.inUse()).isFalse();
// Given that CPU has a large initial allocation:
- acquire(0, 0.99, 0, 0);
+ acquire(0, 0.99, 0);
// When a request for a small CPU allocation is made,
// Then the request fails:
@@ -165,7 +157,7 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- assertThat(acquireNonblocking(0, 0.099, 0, 0)).isNull();
+ assertThat(acquireNonblocking(0, 0.099, 0)).isNull();
}
};
thread2.start();
@@ -178,7 +170,7 @@
assertThat(rm.inUse()).isFalse();
// Given RAM is partially acquired:
- acquire(500, 0, 0, 0);
+ acquire(500, 0, 0);
// When a request for RAM is made that would slightly overallocate RAM,
// Then the request fails:
@@ -186,27 +178,7 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- assertThat(acquireNonblocking(600, 0, 0, 0)).isNull();
- }
- };
- thread1.start();
- thread1.joinAndAssertState(10000);
- }
-
- @Test
- public void testThatIOCannotBeOverallocated() throws Exception {
- assertThat(rm.inUse()).isFalse();
-
- // Given IO is partially acquired:
- acquire(0, 0, 0.5, 0);
-
- // When a request for IO is made that would slightly overallocate IO,
- // Then the request fails:
- TestThread thread1 =
- new TestThread() {
- @Override
- public void runTest() throws Exception {
- assertThat(acquireNonblocking(0, 0, 0.6, 0)).isNull();
+ assertThat(acquireNonblocking(600, 0, 0)).isNull();
}
};
thread1.start();
@@ -218,7 +190,7 @@
assertThat(rm.inUse()).isFalse();
// Given test count is partially acquired:
- acquire(0, 0, 0, 1);
+ acquire(0, 0, 1);
// When a request for tests is made that would slightly overallocate tests,
// Then the request fails:
@@ -226,7 +198,7 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- assertThat(acquireNonblocking(0, 0, 0, 2)).isNull();
+ assertThat(acquireNonblocking(0, 0, 2)).isNull();
}
};
thread1.start();
@@ -237,7 +209,7 @@
public void testHasResources() throws Exception {
assertThat(rm.inUse()).isFalse();
assertThat(rm.threadHasResources()).isFalse();
- acquire(1.0, 0.1, 0.1, 1);
+ acquire(1, 0.1, 1);
assertThat(rm.threadHasResources()).isTrue();
// We have resources in this thread - make sure other threads
@@ -247,28 +219,24 @@
@Override
public void runTest() throws Exception {
assertThat(rm.threadHasResources()).isFalse();
- acquire(1.0, 0, 0, 0);
+ acquire(1, 0, 0);
assertThat(rm.threadHasResources()).isTrue();
- release(1.0, 0, 0, 0);
+ release(1, 0, 0);
assertThat(rm.threadHasResources()).isFalse();
- acquire(0, 0.1, 0, 0);
+ acquire(0, 0.1, 0);
assertThat(rm.threadHasResources()).isTrue();
- release(0, 0.1, 0, 0);
+ release(0, 0.1, 0);
assertThat(rm.threadHasResources()).isFalse();
- acquire(0, 0, 0.1, 0);
+ acquire(0, 0, 1);
assertThat(rm.threadHasResources()).isTrue();
- release(0, 0, 0.1, 0);
- assertThat(rm.threadHasResources()).isFalse();
- acquire(0, 0, 0, 1);
- assertThat(rm.threadHasResources()).isTrue();
- release(0, 0, 0, 1);
+ release(0, 0, 1);
assertThat(rm.threadHasResources()).isFalse();
}
};
thread1.start();
thread1.joinAndAssertState(10000);
- release(1.0, 0.1, 0.1, 1);
+ release(1, 0.1, 1);
assertThat(rm.threadHasResources()).isFalse();
assertThat(rm.inUse()).isFalse();
}
@@ -280,7 +248,7 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- acquire(2000, 2, 0, 0);
+ acquire(2000, 2, 0);
sync.await();
validate(1);
sync.await();
@@ -288,11 +256,11 @@
while (rm.getWaitCount() == 0) {
Thread.yield();
}
- release(2000, 2, 0, 0);
+ release(2000, 2, 0);
assertThat(rm.getWaitCount()).isEqualTo(0);
- acquire(2000, 2, 0, 0); // Will be blocked by the thread2.
+ acquire(2000, 2, 0); // Will be blocked by the thread2.
validate(3);
- release(2000, 2, 0, 0);
+ release(2000, 2, 0);
}
};
TestThread thread2 =
@@ -300,15 +268,15 @@
@Override
public void runTest() throws Exception {
sync2.await();
- assertThat(rm.isAvailable(2000, 2, 0, 0)).isFalse();
- acquire(2000, 2, 0, 0); // Will be blocked by the thread1.
+ assertThat(rm.isAvailable(2000, 2, 0)).isFalse();
+ acquire(2000, 2, 0); // Will be blocked by the thread1.
validate(2);
sync2.await();
// Wait till other thread will be locked.
while (rm.getWaitCount() == 0) {
Thread.yield();
}
- release(2000, 2, 0, 0);
+ release(2000, 2, 0);
}
};
@@ -334,7 +302,7 @@
new TestThread() {
@Override
public void runTest() throws InterruptedException {
- acquire(1, 0, 0, 0);
+ acquire(1, 0, 0);
}
};
smallThread.start();
@@ -345,7 +313,7 @@
public void runTest() {
Thread.currentThread().interrupt();
try {
- acquire(1999, 0, 0, 0);
+ acquire(1999, 0, 0);
fail("Didn't throw interrupted exception");
} catch (InterruptedException e) {
// Expected.
@@ -357,14 +325,13 @@
// This should process the queue. If the request from above is still present, it will take all
// the available memory. But it shouldn't.
rm.setAvailableResources(
- ResourceSet.create(
- /*memoryMb=*/ 2000.0, /*cpuUsage=*/ 1.0, /*ioUsage=*/ 1.0, /* localTestCount= */ 2));
+ ResourceSet.create(/*memoryMb=*/ 2000, /*cpuUsage=*/ 1, /* localTestCount= */ 2));
TestThread thread2 =
new TestThread() {
@Override
public void runTest() throws InterruptedException {
- acquire(1999, 0, 0, 0);
- release(1999, 0, 0, 0);
+ acquire(1999, 0, 0);
+ release(1999, 0, 0);
}
};
thread2.start();
@@ -381,9 +348,9 @@
TestThread thread1 = new TestThread () {
@Override public void runTest() throws Exception {
sync.await();
- acquire(900, 0.5, 0, 0); // Will be blocked by the main thread.
+ acquire(900, 0.5, 0); // Will be blocked by the main thread.
validate(5);
- release(900, 0.5, 0, 0);
+ release(900, 0.5, 0);
sync.await();
}
};
@@ -394,14 +361,14 @@
while (rm.getWaitCount() == 0) {
Thread.yield();
}
- acquire(100, 0.1, 0, 0);
+ acquire(100, 0.1, 0);
validate(2);
- release(100, 0.1, 0, 0);
+ release(100, 0.1, 0);
sync2.await();
- acquire(200, 0.5, 0, 0);
+ acquire(200, 0.5, 0);
validate(4);
sync2.await();
- release(200, 0.5, 0, 0);
+ release(200, 0.5, 0);
}
};
@@ -409,10 +376,10 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- acquire(100, 0.4, 0, 0);
+ acquire(100, 0.4, 0);
sync3.await();
sync3.await();
- release(100, 0.4, 0, 0);
+ release(100, 0.4, 0);
}
};
@@ -420,16 +387,16 @@
new TestThread() {
@Override
public void runTest() throws Exception {
- acquire(750, 0.3, 0, 0);
+ acquire(750, 0.3, 0);
sync4.await();
sync4.await();
- release(750, 0.3, 0, 0);
+ release(750, 0.3, 0);
}
};
// Lock 900 MB, 0.9 CPU in total (spread over three threads so that we can individually release
// parts of it).
- acquire(50, 0.2, 0, 0);
+ acquire(50, 0.2, 0);
thread3.start();
thread4.start();
sync3.await(1, TimeUnit.SECONDS);
@@ -461,7 +428,7 @@
sync.await(1, TimeUnit.SECONDS);
// Release all remaining resources.
- release(50, 0.2, 0, 0);
+ release(50, 0.2, 0);
thread1.join();
thread2.join();
thread3.join();
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceSetTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceSetTest.java
index a5a2ca0..757a718 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ResourceSetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceSetTest.java
@@ -41,7 +41,6 @@
ResourceSet resources = converter.convert("1,0.5,2");
assertThat(resources.getMemoryMb()).isWithin(0.01).of(1.0);
assertThat(resources.getCpuUsage()).isWithin(0.01).of(0.5);
- assertThat(resources.getIoUsage()).isWithin(0.01).of(2.0);
assertThat(resources.getLocalTestCount()).isEqualTo(Integer.MAX_VALUE);
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 91bc6fe..82f461d 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -338,7 +338,7 @@
protected ResourceSet getStartingResources() {
// Effectively disable ResourceManager by default.
- return ResourceSet.createWithRamCpuIo(Double.MAX_VALUE, Double.MAX_VALUE, Double.MAX_VALUE);
+ return ResourceSet.createWithRamCpu(Double.MAX_VALUE, Double.MAX_VALUE);
}
protected final BuildConfigurationCollection createConfigurations(String... args)
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index bc9fa26..5b2daed 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -274,7 +274,7 @@
// Prevent any subprocess execution at all.
SubprocessBuilder.setSubprocessFactory(new SubprocessInterceptor());
resourceManager.setAvailableResources(
- ResourceSet.create(/*memoryMb=*/1, /*cpuUsage=*/1, /*ioUsage=*/1, /*localTestCount=*/1));
+ ResourceSet.create(/*memoryMb=*/1, /*cpuUsage=*/1, /*localTestCount=*/1));
return new InMemoryFileSystem();
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
index e2d013c..89aa14e 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
@@ -466,14 +466,11 @@
.isAtLeast(CppLinkAction.MIN_STATIC_LINK_RESOURCES.getMemoryMb());
assertThat(resources.getCpuUsage())
.isAtLeast(CppLinkAction.MIN_STATIC_LINK_RESOURCES.getCpuUsage());
- assertThat(resources.getIoUsage())
- .isAtLeast(CppLinkAction.MIN_STATIC_LINK_RESOURCES.getIoUsage());
final int linkSize = Iterables.size(linkAction.getLinkCommandLine().getLinkerInputArtifacts());
- ResourceSet scaledSet = ResourceSet.createWithRamCpuIo(
+ ResourceSet scaledSet = ResourceSet.createWithRamCpu(
CppLinkAction.LINK_RESOURCES_PER_INPUT.getMemoryMb() * linkSize,
- CppLinkAction.LINK_RESOURCES_PER_INPUT.getCpuUsage() * linkSize,
- CppLinkAction.LINK_RESOURCES_PER_INPUT.getIoUsage() * linkSize
+ CppLinkAction.LINK_RESOURCES_PER_INPUT.getCpuUsage() * linkSize
);
// Ensure that anything above the minimum is properly scaled.
@@ -481,8 +478,6 @@
|| resources.getMemoryMb() == scaledSet.getMemoryMb()).isTrue();
assertThat(resources.getCpuUsage() == CppLinkAction.MIN_STATIC_LINK_RESOURCES.getCpuUsage()
|| resources.getCpuUsage() == scaledSet.getCpuUsage()).isTrue();
- assertThat(resources.getIoUsage() == CppLinkAction.MIN_STATIC_LINK_RESOURCES.getIoUsage()
- || resources.getIoUsage() == scaledSet.getIoUsage()).isTrue();
}
private CppLinkActionBuilder createLinkBuilder(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index 298aaae..7c02daf 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -144,7 +144,7 @@
options.parse();
inMemoryCache = new InMemoryActionCache();
tsgm = new TimestampGranularityMonitor(clock);
- ResourceManager.instance().setAvailableResources(ResourceSet.createWithRamCpuIo(100, 1, 1));
+ ResourceManager.instance().setAvailableResources(ResourceSet.createWithRamCpu(100, 1));
actions = new HashSet<>();
actionTemplateExpansionFunction = new ActionTemplateExpansionFunction(actionKeyContext);
}
diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
index ccccc61..f9530ca 100644
--- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
@@ -125,7 +125,7 @@
ResourceManager resourceManager = ResourceManager.instanceForTestingOnly();
resourceManager.setAvailableResources(
- ResourceSet.create(/*memoryMb=*/1, /*cpuUsage=*/1, /*ioUsage=*/1, /*localTestCount=*/1));
+ ResourceSet.create(/*memoryMb=*/1, /*cpuUsage=*/1, /*localTestCount=*/1));
Path execRoot = directories.getExecRoot(TestConstants.WORKSPACE_NAME);
this.executor =
new BlazeExecutor(
