From 034a2815a1e18be5c8b36c6a78f44bb849dff437 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 28 Feb 2023 19:13:21 +0100 Subject: [PATCH] Report background download for BwoB (#17619) When building without the bytes, downloads can happen when Bazel need to fetch inputs for local actions, download outputs of toplevel targets, and etc. The downloads happen silently at the background. This PR makes output downloads of toplevel targets visible to the UI. For downloads during the build, the UI looks like: ``` [2 / 5] 2 actions, 1 running [Prepa] Executing genrule //:foo Executing genrule //:bar; 2s Fetching bazel-out/darwin-fastbuild/bin/baz; 50.8 MiB / 1.0 GiB; 4s ``` For downloads after build is completed, the UI looks like: ``` INFO: 2 processes: 1 remote cache hit, 1 internal. INFO: Build completed successfully, 2 total actions Fetching bazel-out/darwin-fastbuild/bin/baz; 48.8 MiB / 1.0 GiB ``` Fixes https://github.com/bazelbuild/bazel/issues/17417. Closes #17604. PiperOrigin-RevId: 512934756 Change-Id: I502489420ab9b84efb74ceabbe3dd4cb4a7a62e2 --- .../build/lib/exec/SpawnProgressEvent.java | 6 +-- .../remote/AbstractActionInputPrefetcher.java | 15 ++++-- .../lib/remote/RemoteActionInputFetcher.java | 49 ++++++++++++++++++- .../build/lib/remote/RemoteCache.java | 26 ++++++++-- .../remote/common/ProgressStatusListener.java | 3 +- .../build/lib/runtime/UiStateTracker.java | 11 +++-- .../remote/ActionInputPrefetcherTestBase.java | 10 ++-- 7 files changed, 98 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnProgressEvent.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnProgressEvent.java index 9ddde03a1e1497..4f4b139a576be5 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnProgressEvent.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnProgressEvent.java @@ -28,13 +28,13 @@ public static SpawnProgressEvent create(String resourceId, String progress, bool } /** The id that uniquely determines the progress among all progress events for this spawn. */ - abstract String progressId(); + public abstract String progressId(); /** Human readable description of the progress. */ - abstract String progress(); + public abstract String progress(); /** Whether the progress reported about is finished already. */ - abstract boolean finished(); + public abstract boolean finished(); @Override public void postTo(ExtendedEventHandler eventHandler, ActionExecutionMetadata action) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index 478d30d5b4c44e..bcdf604b2f81bc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -157,7 +157,11 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) { * @param tempPath the temporary path which the input should be written to. */ protected abstract ListenableFuture doDownloadFile( - Path tempPath, PathFragment execPath, FileArtifactValue metadata, Priority priority) + Reporter reporter, + Path tempPath, + PathFragment execPath, + FileArtifactValue metadata, + Priority priority) throws IOException; protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOException {} @@ -309,7 +313,8 @@ private Completable prefetchInputTree( return toTransferResult( toCompletable( () -> - doDownloadFile(tempPath, treeFile.getExecPath(), metadata, priority), + doDownloadFile( + reporter, tempPath, treeFile.getExecPath(), metadata, priority), directExecutor())); }); @@ -455,7 +460,11 @@ private Completable downloadFileNoCheckRx( toCompletable( () -> doDownloadFile( - tempPath, finalPath.relativeTo(execRoot), metadata, priority), + reporter, + tempPath, + finalPath.relativeTo(execRoot), + metadata, + priority), directExecutor()) .doOnComplete( () -> { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java index bd482a2d9da823..bcda6374686676 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.devtools.build.lib.remote.common.ProgressStatusListener.NO_ACTION; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; @@ -24,7 +25,10 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput; +import com.google.devtools.build.lib.events.ExtendedEventHandler.FetchProgress; import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.exec.SpawnProgressEvent; +import com.google.devtools.build.lib.remote.RemoteCache.DownloadProgressReporter; import com.google.devtools.build.lib.remote.common.BulkTransferException; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -90,7 +94,11 @@ protected boolean canDownloadFile(Path path, FileArtifactValue metadata) { @Override protected ListenableFuture doDownloadFile( - Path tempPath, PathFragment execPath, FileArtifactValue metadata, Priority priority) + Reporter reporter, + Path tempPath, + PathFragment execPath, + FileArtifactValue metadata, + Priority priority) throws IOException { checkArgument(metadata.isRemote(), "Cannot download file that is not a remote file."); RequestMetadata requestMetadata = @@ -98,7 +106,44 @@ protected ListenableFuture doDownloadFile( RemoteActionExecutionContext context = RemoteActionExecutionContext.create(requestMetadata); Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); - return remoteCache.downloadFile(context, tempPath, digest); + + DownloadProgressReporter downloadProgressReporter; + if (priority == Priority.LOW) { + // Only report download progress for toplevel outputs + downloadProgressReporter = + new DownloadProgressReporter( + /* includeFile= */ false, + progress -> reporter.post(new DownloadProgress(progress)), + execPath.toString(), + metadata.getSize()); + } else { + downloadProgressReporter = new DownloadProgressReporter(NO_ACTION, "", 0); + } + + return remoteCache.downloadFile(context, tempPath, digest, downloadProgressReporter); + } + + public static class DownloadProgress implements FetchProgress { + private final SpawnProgressEvent progress; + + public DownloadProgress(SpawnProgressEvent progress) { + this.progress = progress; + } + + @Override + public String getResourceIdentifier() { + return progress.progressId(); + } + + @Override + public String getProgress() { + return progress.progress(); + } + + @Override + public boolean isFinished() { + return progress.finished(); + } } @Override diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index f081add8ad9caa..1fe79e16906f0b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -248,6 +248,7 @@ private ListenableFuture downloadBlob( /** A reporter that reports download progresses. */ public static class DownloadProgressReporter { private static final Pattern PATTERN = Pattern.compile("^bazel-out/[^/]+/[^/]+/"); + private final boolean includeFile; private final ProgressStatusListener listener; private final String id; private final String file; @@ -255,6 +256,12 @@ public static class DownloadProgressReporter { private final AtomicLong downloadedBytes = new AtomicLong(0); public DownloadProgressReporter(ProgressStatusListener listener, String file, long totalSize) { + this(/* includeFile= */ true, listener, file, totalSize); + } + + public DownloadProgressReporter( + boolean includeFile, ProgressStatusListener listener, String file, long totalSize) { + this.includeFile = includeFile; this.listener = listener; this.id = file; this.totalSize = bytesCountToDisplayString(totalSize); @@ -279,12 +286,21 @@ void finished() { private void reportProgress(boolean includeBytes, boolean finished) { String progress; if (includeBytes) { - progress = - String.format( - "Downloading %s, %s / %s", - file, bytesCountToDisplayString(downloadedBytes.get()), totalSize); + if (includeFile) { + progress = + String.format( + "Downloading %s, %s / %s", + file, bytesCountToDisplayString(downloadedBytes.get()), totalSize); + } else { + progress = + String.format("%s / %s", bytesCountToDisplayString(downloadedBytes.get()), totalSize); + } } else { - progress = String.format("Downloading %s", file); + if (includeFile) { + progress = String.format("Downloading %s", file); + } else { + progress = ""; + } } listener.onProgressStatus(SpawnProgressEvent.create(id, progress, finished)); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/ProgressStatusListener.java b/src/main/java/com/google/devtools/build/lib/remote/common/ProgressStatusListener.java index df5a8beae54758..55b4b57f7c669f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/ProgressStatusListener.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/ProgressStatusListener.java @@ -13,13 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.remote.common; +import com.google.devtools.build.lib.exec.SpawnProgressEvent; import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; /** An interface that is used to receive {@link ProgressStatus} updates during spawn execution. */ @FunctionalInterface public interface ProgressStatusListener { - void onProgressStatus(ProgressStatus progress); + void onProgressStatus(SpawnProgressEvent progress); /** A {@link ProgressStatusListener} that does nothing. */ ProgressStatusListener NO_ACTION = diff --git a/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java b/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java index 116e25bb8e6a41..dac8c42ed3b7bc 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java @@ -993,7 +993,8 @@ synchronized boolean hasActivities() { return !(buildCompleted() && bepOpenTransports.isEmpty() && activeActionUploads.get() == 0 - && activeActionDownloads.get() == 0); + && activeActionDownloads.get() == 0 + && runningDownloads.isEmpty()); } /** @@ -1106,7 +1107,8 @@ private void reportOnOneDownload( terminalWriter.append(url + postfix); } - protected void reportOnDownloads(AnsiTerminalWriter terminalWriter) throws IOException { + protected void reportOnDownloads(PositionAwareAnsiTerminalWriter terminalWriter) + throws IOException { int count = 0; long nanoTime = clock.nanoTime(); int downloadCount = runningDownloads.size(); @@ -1116,7 +1118,10 @@ protected void reportOnDownloads(AnsiTerminalWriter terminalWriter) throws IOExc break; } count++; - terminalWriter.newline().append(FETCH_PREFIX); + if (terminalWriter.getPosition() != 0) { + terminalWriter.newline(); + } + terminalWriter.append(FETCH_PREFIX); reportOnOneDownload( url, nanoTime, diff --git a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java index fae2d964f9ef04..68232c3918dbe6 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java @@ -173,7 +173,7 @@ public void prefetchFiles_fileExists_doNotDownload() wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); - verify(prefetcher, never()).doDownloadFile(any(), any(), any(), any()); + verify(prefetcher, never()).doDownloadFile(any(), any(), any(), any(), any()); assertThat(prefetcher.downloadedFiles()).containsExactly(a.getPath()); assertThat(prefetcher.downloadsInProgress()).isEmpty(); } @@ -190,7 +190,7 @@ public void prefetchFiles_fileExistsButContentMismatches_download() wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); - verify(prefetcher).doDownloadFile(any(), eq(a.getExecPath()), any(), any()); + verify(prefetcher).doDownloadFile(any(), any(), eq(a.getExecPath()), any(), any()); assertThat(prefetcher.downloadedFiles()).containsExactly(a.getPath()); assertThat(prefetcher.downloadsInProgress()).isEmpty(); assertThat(FileSystemUtils.readContent(a.getPath(), UTF_8)).isEqualTo("hello world remote"); @@ -531,8 +531,8 @@ protected static void mockDownload( throws IOException { doAnswer( invocation -> { - Path path = invocation.getArgument(0); - FileArtifactValue metadata = invocation.getArgument(2); + Path path = invocation.getArgument(1); + FileArtifactValue metadata = invocation.getArgument(3); byte[] content = cas.get(HashCode.fromBytes(metadata.getDigest())); if (content == null) { return Futures.immediateFailedFuture(new IOException("Not found")); @@ -541,7 +541,7 @@ protected static void mockDownload( return resultSupplier.get(); }) .when(prefetcher) - .doDownloadFile(any(), any(), any(), any()); + .doDownloadFile(any(), any(), any(), any(), any()); } private void assertReadableNonWritableAndExecutable(Path path) throws IOException {