Skip to content

Commit

Permalink
Add TTL to remote metadata
Browse files Browse the repository at this point in the history 
and use it to when checking filesystem dirtiness in an incremental build.
  • Loading branch information
@coeuvre
coeuvre committed Nov 9, 2022
1 parent 99a8186 commit b541120
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 33 deletions.
43 changes: 33 additions & 10 deletions src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,14 @@ public boolean isRemote() {
return false;
}

/**
* Returns {@code true} if the file is remote and is available remotely at time {@code epochMilli}
* which is the milliseconds since epoch.
*/
public boolean isRemotelyAvailable(long epochMilli) {
return false;
}

/**
* Provides a best-effort determination whether the file was changed since the digest was
* computed. This method performs file system I/O, so may be expensive. It's primarily intended to
Expand Down Expand Up @@ -551,34 +559,37 @@ public static class RemoteFileArtifactValue extends FileArtifactValue {
protected final long size;
protected final int locationIndex;
protected final String actionId;
protected final long expiredAtEpochMilli;

private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) {
private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId, long expiredAtEpochMilli) {
this.digest = Preconditions.checkNotNull(digest, actionId);
this.size = size;
this.locationIndex = locationIndex;
this.actionId = actionId;
this.expiredAtEpochMilli = expiredAtEpochMilli;
}

public static RemoteFileArtifactValue create(
byte[] digest, long size, int locationIndex, String actionId) {
return new RemoteFileArtifactValue(digest, size, locationIndex, actionId);
byte[] digest, long size, int locationIndex, String actionId, long expiredAtEpochMilli) {
return new RemoteFileArtifactValue(digest, size, locationIndex, actionId, expiredAtEpochMilli);
}

public static RemoteFileArtifactValue create(byte[] digest, long size, int locationIndex) {
return new RemoteFileArtifactValue(digest, size, locationIndex, /* actionId= */ "");
public static RemoteFileArtifactValue create(byte[] digest, long size, int locationIndex, long expiredAtEpochMilli) {
return new RemoteFileArtifactValue(digest, size, locationIndex, /* actionId= */ "", expiredAtEpochMilli);
}

public static RemoteFileArtifactValue create(
byte[] digest,
long size,
int locationIndex,
String actionId,
long expiredAtEpochMilli,
@Nullable PathFragment materializationExecPath) {
if (materializationExecPath != null) {
return new RemoteFileArtifactValueWithMaterializationPath(
digest, size, locationIndex, actionId, materializationExecPath);
digest, size, locationIndex, actionId, expiredAtEpochMilli, materializationExecPath);
}
return new RemoteFileArtifactValue(digest, size, locationIndex, actionId);
return new RemoteFileArtifactValue(digest, size, locationIndex, actionId, expiredAtEpochMilli);
}

@Override
Expand All @@ -591,12 +602,13 @@ public boolean equals(Object o) {
return Arrays.equals(digest, that.digest)
&& size == that.size
&& locationIndex == that.locationIndex
&& Objects.equals(actionId, that.actionId);
&& Objects.equals(actionId, that.actionId)
&& expiredAtEpochMilli == that.expiredAtEpochMilli;
}

@Override
public int hashCode() {
return Objects.hash(Arrays.hashCode(digest), size, locationIndex, actionId);
return Objects.hash(Arrays.hashCode(digest), size, locationIndex, actionId, expiredAtEpochMilli);
}

@Override
Expand Down Expand Up @@ -624,6 +636,10 @@ public String getActionId() {
return actionId;
}

public long getExpiredAtEpochMilli() {
return expiredAtEpochMilli;
}

@Override
public long getModifiedTime() {
throw new UnsupportedOperationException(
Expand All @@ -645,13 +661,19 @@ public boolean isRemote() {
return true;
}

@Override
public boolean isRemotelyAvailable(long epochMilli) {
return epochMilli < expiredAtEpochMilli;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("digest", bytesToString(digest))
.add("size", size)
.add("locationIndex", locationIndex)
.add("actionId", actionId)
.add("expiredAtEpochMilli", expiredAtEpochMilli)
.toString();
}
}
Expand All @@ -671,8 +693,9 @@ private RemoteFileArtifactValueWithMaterializationPath(
long size,
int locationIndex,
String actionId,
long expiredAtEpochMilli,
PathFragment materializationExecPath) {
super(digest, size, locationIndex, actionId);
super(digest, size, locationIndex, actionId, expiredAtEpochMilli);
this.materializationExecPath = Preconditions.checkNotNull(materializationExecPath);
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(
}

return RemoteFileArtifactValue.create(
digest, size, locationIndex, actionId, materializationExecPath);
digest, size, locationIndex, actionId, 0, materializationExecPath);
}

/** @return action data encoded as a byte[] array. */
Expand Down
20 changes: 14 additions & 6 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,12 @@ public void updateContext(MetadataInjector metadataInjector) {
this.metadataInjector = metadataInjector;
}

void injectRemoteFile(PathFragment path, byte[] digest, long size, String actionId)
void injectRemoteFile(PathFragment path, byte[] digest, long size, String actionId, long expireAtEpochMilli)
throws IOException {
if (!isOutput(path)) {
return;
}
remoteOutputTree.injectRemoteFile(path, digest, size, actionId);
remoteOutputTree.injectRemoteFile(path, digest, size, actionId, expireAtEpochMilli);
}

void flush() throws IOException {
Expand Down Expand Up @@ -242,6 +242,7 @@ private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact ou
metadata.getSize(),
metadata.getLocationIndex(),
metadata.getActionId(),
metadata.getExpiredAtEpochMilli(),
// Avoid a double indirection when the target is already materialized as a symlink.
metadata.getMaterializationExecPath().orElse(targetPath.relativeTo(execRoot)));

Expand All @@ -256,7 +257,8 @@ private RemoteFileArtifactValue createRemoteMetadata(RemoteFileInfo remoteFile)
remoteFile.getFastDigest(),
remoteFile.getSize(),
/* locationIndex= */ 1,
remoteFile.getActionId());
remoteFile.getActionId(),
remoteFile.getExpireAtEpochMilli());
}

@Override
Expand Down Expand Up @@ -713,7 +715,7 @@ protected FileInfo newFile(Clock clock, PathFragment path) {
return new RemoteFileInfo(clock);
}

void injectRemoteFile(PathFragment path, byte[] digest, long size, String actionId)
void injectRemoteFile(PathFragment path, byte[] digest, long size, String actionId, long expireAtEpochMilli)
throws IOException {
createDirectoryAndParents(path.getParentDirectory());
InMemoryContentInfo node = getOrCreateWritableInode(path);
Expand All @@ -724,7 +726,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size, String action
}

RemoteFileInfo remoteFileInfo = (RemoteFileInfo) node;
remoteFileInfo.set(digest, size, actionId);
remoteFileInfo.set(digest, size, actionId, expireAtEpochMilli);
}

@Nullable
Expand All @@ -742,15 +744,17 @@ static class RemoteFileInfo extends FileInfo {
private byte[] digest;
private long size;
private String actionId;
private long expireAtEpochMilli;

RemoteFileInfo(Clock clock) {
super(clock);
}

private void set(byte[] digest, long size, String actionId) {
private void set(byte[] digest, long size, String actionId, long expireAtEpochMilli) {
this.digest = digest;
this.size = size;
this.actionId = actionId;
this.expireAtEpochMilli = expireAtEpochMilli;
}

@Override
Expand Down Expand Up @@ -781,5 +785,9 @@ public long getSize() {
public String getActionId() {
return actionId;
}

public long getExpireAtEpochMilli() {
return expireAtEpochMilli;
}
}
}
10 changes: 6 additions & 4 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ private void createSymlinks(Iterable<SymlinkMetadata> symlinks) throws IOExcepti
}
}

private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata metadata)
private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata metadata, long expireAtEpochMilli)
throws IOException {
FileSystem actionFileSystem = action.getSpawnExecutionContext().getActionFileSystem();
checkState(actionFileSystem instanceof RemoteActionFileSystem);
Expand All @@ -780,7 +780,8 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
file.path().asFragment(),
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes(),
context.getRequestMetadata().getActionId());
context.getRequestMetadata().getActionId(),
expireAtEpochMilli);
}
}

Expand All @@ -789,7 +790,8 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
file.path().asFragment(),
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes(),
context.getRequestMetadata().getActionId());
context.getRequestMetadata().getActionId(),
expireAtEpochMilli);
}
}

Expand Down Expand Up @@ -1118,7 +1120,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
}
}

injectRemoteArtifacts(action, metadata);
injectRemoteArtifacts(action, metadata, 0);

try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) {
if (inMemoryOutput != null) {
Expand Down
37 changes: 25 additions & 12 deletions src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.WalkableGraph;
import java.io.IOException;
import java.time.Instant;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -228,6 +229,7 @@ public NavigableSet<PathFragment> get() {
}
});

long epochMilli = Instant.now().toEpochMilli();
boolean interrupted;
try (SilentCloseable c = Profiler.instance().profile("getDirtyActionValues.stat_files")) {
for (List<Pair<SkyKey, ActionExecutionValue>> shard : outputShards) {
Expand All @@ -239,15 +241,17 @@ public NavigableSet<PathFragment> get() {
knownModifiedOutputFiles,
sortedKnownModifiedOutputFiles,
trustRemoteArtifacts,
modifiedOutputsReceiver)
modifiedOutputsReceiver,
epochMilli)
: batchStatJob(
dirtyKeys,
shard,
batchStatter,
knownModifiedOutputFiles,
sortedKnownModifiedOutputFiles,
trustRemoteArtifacts,
modifiedOutputsReceiver);
modifiedOutputsReceiver,
epochMilli);
executor.execute(job);
}

Expand All @@ -273,7 +277,8 @@ private Runnable batchStatJob(
ImmutableSet<PathFragment> knownModifiedOutputFiles,
Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles,
boolean trustRemoteArtifacts,
ModifiedOutputsReceiver modifiedOutputsReceiver) {
ModifiedOutputsReceiver modifiedOutputsReceiver,
long epochMilli) {
return () -> {
Map<Artifact, Pair<SkyKey, ActionExecutionValue>> fileToKeyAndValue = new HashMap<>();
Map<Artifact, Pair<SkyKey, ActionExecutionValue>> treeArtifactsToKeyAndValue =
Expand Down Expand Up @@ -331,7 +336,8 @@ && shouldCheckFile(knownModifiedOutputFiles, artifact)) {
knownModifiedOutputFiles,
sortedKnownModifiedOutputFiles,
trustRemoteArtifacts,
modifiedOutputsReceiver)
modifiedOutputsReceiver,
epochMilli)
.run();
return;
} catch (InterruptedException e) {
Expand Down Expand Up @@ -393,7 +399,8 @@ private Runnable outputStatJob(
ImmutableSet<PathFragment> knownModifiedOutputFiles,
Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles,
boolean trustRemoteArtifacts,
ModifiedOutputsReceiver modifiedOutputsReceiver) {
ModifiedOutputsReceiver modifiedOutputsReceiver,
long epochMilli) {
return new Runnable() {
@Override
public void run() {
Expand All @@ -405,7 +412,8 @@ public void run() {
knownModifiedOutputFiles,
sortedKnownModifiedOutputFiles,
trustRemoteArtifacts,
modifiedOutputsReceiver)) {
modifiedOutputsReceiver,
epochMilli)) {
dirtyKeys.add(keyAndValue.getFirst());
}
}
Expand Down Expand Up @@ -441,7 +449,8 @@ private boolean artifactIsDirtyWithDirectSystemCalls(
ImmutableSet<PathFragment> knownModifiedOutputFiles,
boolean trustRemoteArtifacts,
Map.Entry<? extends Artifact, FileArtifactValue> entry,
ModifiedOutputsReceiver modifiedOutputsReceiver) {
ModifiedOutputsReceiver modifiedOutputsReceiver,
long epochMilli) {
Artifact file = entry.getKey();
FileArtifactValue lastKnownData = entry.getValue();
if (file.isMiddlemanArtifact() || !shouldCheckFile(knownModifiedOutputFiles, file)) {
Expand All @@ -453,7 +462,8 @@ private boolean artifactIsDirtyWithDirectSystemCalls(
boolean trustRemoteValue =
fileMetadata.getType() == FileStateType.NONEXISTENT
&& lastKnownData.isRemote()
&& trustRemoteArtifacts;
&& trustRemoteArtifacts
&& lastKnownData.isRemotelyAvailable(epochMilli);
if (!trustRemoteValue && fileMetadata.couldBeModifiedSince(lastKnownData)) {
modifiedOutputsReceiver.reportModifiedOutputFile(
fileMetadata.getType() != FileStateType.NONEXISTENT
Expand All @@ -475,11 +485,12 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(
ImmutableSet<PathFragment> knownModifiedOutputFiles,
Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles,
boolean trustRemoteArtifacts,
ModifiedOutputsReceiver modifiedOutputsReceiver) {
ModifiedOutputsReceiver modifiedOutputsReceiver,
long epochMilli) {
boolean isDirty = false;
for (Map.Entry<Artifact, FileArtifactValue> entry : actionValue.getAllFileValues().entrySet()) {
if (artifactIsDirtyWithDirectSystemCalls(
knownModifiedOutputFiles, trustRemoteArtifacts, entry, modifiedOutputsReceiver)) {
knownModifiedOutputFiles, trustRemoteArtifacts, entry, modifiedOutputsReceiver, epochMilli)) {
isDirty = true;
}
}
Expand All @@ -495,7 +506,8 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(
knownModifiedOutputFiles,
trustRemoteArtifacts,
childEntry,
modifiedOutputsReceiver)) {
modifiedOutputsReceiver,
epochMilli)) {
isDirty = true;
}
}
Expand All @@ -510,7 +522,8 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(
Maps.immutableEntry(
archivedRepresentation.archivedTreeFileArtifact(),
archivedRepresentation.archivedFileValue()),
modifiedOutputsReceiver))
modifiedOutputsReceiver,
epochMilli))
.orElse(false);
}

Expand Down

0 comments on commit b541120

Please sign in to comment.