Skip to content

Commit 84a97fc

Browse files
committed
Use FileArtifactValue#setContentsProxy for remote repo contents cache files
Even though expiration times don't matter in Bazel (they aren't honored), supporting the contents proxy optimization avoids Skyframe invalidation when external repo files are materialized later.
1 parent cb10d4f commit 84a97fc

File tree

3 files changed

+31
-7
lines changed

3 files changed

+31
-7
lines changed

src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,11 @@ public static FileArtifactValue createForInlineFile(byte[] bytes, HashFunction h
395395
return new InlineFileArtifactValue(bytes, hashFunction.hashBytes(bytes).asBytes());
396396
}
397397

398+
/**
399+
* Prefer {@link #createForRemoteFileWithMaterializationData} if the remote file may be
400+
* materialized in the local filesystem at a later point as this variant supports {@link
401+
* #setContentsProxy}.
402+
*/
398403
public static FileArtifactValue createForRemoteFile(byte[] digest, long size, int locationIndex) {
399404
return new RemoteFileArtifactValue(digest, size, locationIndex);
400405
}

src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@
6060
import java.io.InterruptedIOException;
6161
import java.io.OutputStream;
6262
import java.nio.channels.SeekableByteChannel;
63+
import java.time.Duration;
64+
import java.time.Instant;
6365
import java.util.ArrayList;
6466
import java.util.Collection;
6567
import java.util.List;
@@ -98,6 +100,7 @@ public final class RemoteExternalOverlayFileSystem extends FileSystem {
98100
@Nullable private String buildRequestId;
99101
@Nullable private String commandId;
100102
@Nullable private MemoizingEvaluator evaluator;
103+
@Nullable private Duration remoteCacheTtl;
101104
@Nullable private ExecutorService materializationExecutor;
102105

103106
public RemoteExternalOverlayFileSystem(PathFragment externalDirectory, FileSystem nativeFs) {
@@ -115,21 +118,24 @@ public void beforeCommand(
115118
Reporter reporter,
116119
String buildRequestId,
117120
String commandId,
118-
MemoizingEvaluator evaluator) {
121+
MemoizingEvaluator evaluator,
122+
Duration remoteCacheTtl) {
119123
checkState(
120124
this.cache == null
121125
&& this.inputPrefetcher == null
122126
&& this.reporter == null
123127
&& this.buildRequestId == null
124128
&& this.commandId == null
125129
&& this.evaluator == null
130+
&& this.remoteCacheTtl == null
126131
&& this.materializationExecutor == null);
127132
this.cache = cache;
128133
this.inputPrefetcher = inputPrefetcher;
129134
this.reporter = reporter;
130135
this.buildRequestId = buildRequestId;
131136
this.commandId = commandId;
132137
this.evaluator = evaluator;
138+
this.remoteCacheTtl = remoteCacheTtl;
133139
this.materializationExecutor =
134140
Executors.newThreadPerTaskExecutor(
135141
Thread.ofVirtual().name("remote-repo-materialization-", 0).factory());
@@ -146,6 +152,7 @@ public void afterCommand() {
146152
this.reporter = null;
147153
this.buildRequestId = null;
148154
this.commandId = null;
155+
this.remoteCacheTtl = null;
149156
// Materializations happen synchronously and upon request by other repo rules, so there is no
150157
// reason to await their orderly completion in afterCommand.
151158
materializationExecutor.shutdownNow();
@@ -193,7 +200,12 @@ public boolean injectRemoteRepo(RepositoryName repo, Tree remoteContents, String
193200
var repoDir = externalDirectory.getChild(repo.getName());
194201
var filesToPrefetch = new ArrayList<PathFragment>();
195202
injectRecursively(
196-
externalFs, repoDir, remoteContents.getRoot(), childMap, filesToPrefetch::add);
203+
externalFs,
204+
repoDir,
205+
remoteContents.getRoot(),
206+
childMap,
207+
filesToPrefetch::add,
208+
Instant.now().plus(remoteCacheTtl));
197209
try {
198210
// TODO: This prefetches a large number of small files. Investigate whether BatchReadBlobs
199211
// would be more efficient.
@@ -220,7 +232,8 @@ private static void injectRecursively(
220232
PathFragment path,
221233
Directory dir,
222234
ImmutableMap<Digest, Directory> childMap,
223-
Consumer<PathFragment> filesToPrefetch)
235+
Consumer<PathFragment> filesToPrefetch,
236+
Instant expirationTime)
224237
throws IOException {
225238
fs.createDirectoryAndParents(path);
226239
for (var file : dir.getFilesList()) {
@@ -230,10 +243,15 @@ private static void injectRecursively(
230243
}
231244
fs.injectFile(
232245
filePath,
233-
FileArtifactValue.createForRemoteFile(
246+
// Using the *WithMaterializationData variant ensures that the file benefits from the
247+
// FileContentsProxy optimization to avoid widespread invalidation when it is
248+
// materialized later, even if expiration times aren't relevant (depends on the usage
249+
// of the lease extension).
250+
FileArtifactValue.createForRemoteFileWithMaterializationData(
234251
DigestUtil.toBinaryDigest(file.getDigest()),
235252
file.getDigest().getSizeBytes(),
236-
/* locationIndex= */ 1));
253+
/* locationIndex= */ 1,
254+
expirationTime));
237255
fs.setExecutable(filePath, file.getIsExecutable());
238256
// The RE API does not track whether a file is readable or writable. We choose to make all
239257
// files readable and not writable to ensure that other repo rules can't accidentally modify
@@ -253,7 +271,7 @@ private static void injectRecursively(
253271
"Directory %s with digest %s not found in tree"
254272
.formatted(subdirPath, subdirNode.getDigest().getHash()));
255273
}
256-
injectRecursively(fs, subdirPath, subdir, childMap, filesToPrefetch);
274+
injectRecursively(fs, subdirPath, subdir, childMap, filesToPrefetch, expirationTime);
257275
}
258276
}
259277

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -810,7 +810,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
810810
env.getReporter(),
811811
buildRequestId,
812812
invocationId,
813-
env.getSkyframeExecutor().getEvaluator());
813+
env.getSkyframeExecutor().getEvaluator(),
814+
remoteOptions.remoteCacheTtl);
814815
}
815816

816817
buildEventArtifactUploaderFactoryDelegate.init(

0 commit comments

Comments
 (0)