From d6e204c29ed2d8f99cb4b838ab84198c6789d8c9 Mon Sep 17 00:00:00 2001 From: Roman Salvador Date: Mon, 28 Oct 2024 09:57:52 -0700 Subject: [PATCH] Prevent excessive thread creation in disk cache garbage collection Fixes https://github.com/bazelbuild/bazel/issues/24098 With this change the disk cache garbage collection works correctly: ``` 241027 09:06:34.732:I 681 [com.google.devtools.build.lib.remote.disk.DiskCacheGarbageCollectorIdleTask.run] Disk cache garbage collection started 241027 09:07:06.123:I 681 [com.google.devtools.build.lib.remote.disk.DiskCacheGarbageCollectorIdleTask.run] Deleted 190243 of 446229 files, reclaimed 5.4 GiB of 15.4 GiB ``` Closes #24099. PiperOrigin-RevId: 690652512 Change-Id: Ie8d1fa6b2afb0bd5bd85fdb6835871023a64ad24 --- .../disk/DiskCacheGarbageCollector.java | 17 ++++++++-- .../DiskCacheGarbageCollectorIdleTask.java | 3 +- .../disk/DiskCacheGarbageCollectorTest.java | 32 +++++++++++++------ 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java index a57c2bf5fff105..5e19689cbec326 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java @@ -121,16 +121,24 @@ public record CollectionStats( long totalBytes, long deletedEntries, long deletedBytes, - boolean concurrentUpdate) { + boolean concurrentUpdate, + Duration elapsedTime) { /** Returns a human-readable summary. */ public String displayString() { - return "Deleted %d of %d files, reclaimed %s of %s%s" + double elapsedSeconds = elapsedTime.toSecondsPart() + elapsedTime.toMillisPart() / 1000.0; + int filesPerSecond = (int) Math.round((double) deletedEntries / elapsedSeconds); + int mbPerSecond = (int) Math.round((deletedBytes / (1024.0 * 1024.0)) / elapsedSeconds); + + return "Deleted %d of %d files, reclaimed %s of %s in %.2f seconds (%d files/s, %d MB/s)%s" .formatted( deletedEntries(), totalEntries(), bytesCountToDisplayString(deletedBytes()), bytesCountToDisplayString(totalBytes()), + elapsedSeconds, + filesPerSecond, + mbPerSecond, concurrentUpdate() ? " (concurrent update detected)" : ""); } } @@ -180,6 +188,7 @@ public CollectionStats run() throws IOException, InterruptedException { } private CollectionStats runUnderLock() throws IOException, InterruptedException { + Instant startTime = Instant.now(); EntryScanner scanner = new EntryScanner(); EntryDeleter deleter = new EntryDeleter(); @@ -191,13 +200,15 @@ private CollectionStats runUnderLock() throws IOException, InterruptedException } DeletionStats deletionStats = deleter.await(); + Duration elapsedTime = Duration.between(startTime, Instant.now()); return new CollectionStats( allEntries.size(), allEntries.stream().mapToLong(Entry::size).sum(), deletionStats.deletedEntries(), deletionStats.deletedBytes(), - deletionStats.concurrentUpdate()); + deletionStats.concurrentUpdate(), + elapsedTime); } /** Lists all disk cache entries, performing I/O in parallel. */ diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java index 7c03e16fbb6b3c..b5ebaae5d91219 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java @@ -36,7 +36,8 @@ public final class DiskCacheGarbageCollectorIdleTask implements IdleTask { private final DiskCacheGarbageCollector gc; private static final ExecutorService executorService = - Executors.newCachedThreadPool( + Executors.newFixedThreadPool( + Math.max(4, Runtime.getRuntime().availableProcessors()), new ThreadFactoryBuilder().setNameFormat("disk-cache-gc-%d").build()); private DiskCacheGarbageCollectorIdleTask(Duration delay, DiskCacheGarbageCollector gc) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java index 41b778aacb3d98..9e164c62897541 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java @@ -61,7 +61,7 @@ public void sizePolicy_noCollection() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.empty()); - assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false)); + assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false, Duration.ZERO)); assertFilesExist("ac/123", "cas/456"); } @@ -75,7 +75,8 @@ public void sizePolicy_collectsOldest() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.empty()); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); + assertThat(stats) + .isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false, Duration.ZERO)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -90,7 +91,8 @@ public void sizePolicy_tieBreakByPath() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.empty()); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); + assertThat(stats) + .isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false, Duration.ZERO)); assertFilesExist("cas/456", "cas/def"); assertFilesDoNotExist("ac/123", "ac/abc"); } @@ -103,7 +105,7 @@ public void agePolicy_noCollection() throws Exception { CollectionStats stats = runGarbageCollector(Optional.empty(), Optional.of(days(3))); - assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false)); + assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false, Duration.ZERO)); assertFilesExist("ac/123", "cas/456"); } @@ -117,7 +119,8 @@ public void agePolicy_collectsOldest() throws Exception { CollectionStats stats = runGarbageCollector(Optional.empty(), Optional.of(Duration.ofDays(3))); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); + assertThat(stats) + .isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false, Duration.ZERO)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -130,7 +133,7 @@ public void sizeAndAgePolicy_noCollection() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.of(days(1))); - assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false)); + assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false, Duration.ZERO)); assertFilesExist("ac/123", "cas/456"); } @@ -144,7 +147,8 @@ public void sizeAndAgePolicy_sizeMoreRestrictiveThanAge_collectsOldest() throws CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.of(days(4))); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); + assertThat(stats) + .isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false, Duration.ZERO)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -159,7 +163,8 @@ public void sizeAndAgePolicy_ageMoreRestrictiveThanSize_collectsOldest() throws CollectionStats stats = runGarbageCollector(Optional.of(kbytes(3)), Optional.of(days(3))); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); + assertThat(stats) + .isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false, Duration.ZERO)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -171,7 +176,7 @@ public void ignoresTmpAndGcSubdirectories() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(1L), Optional.of(days(1))); - assertThat(stats).isEqualTo(new CollectionStats(0, 0, 0, 0, false)); + assertThat(stats).isEqualTo(new CollectionStats(0, 0, 0, 0, false, Duration.ZERO)); assertFilesExist("gc/foo", "tmp/foo"); } @@ -212,7 +217,14 @@ private CollectionStats runGarbageCollector( rootDir, executorService, new DiskCacheGarbageCollector.CollectionPolicy(maxSizeBytes, maxAge)); - return gc.run(); + CollectionStats resultStats = gc.run(); + return new CollectionStats( + resultStats.totalEntries(), + resultStats.totalBytes(), + resultStats.deletedEntries(), + resultStats.deletedBytes(), + resultStats.concurrentUpdate(), + Duration.ZERO); } private void writeFiles(Entry... entries) throws IOException {