Skip to content

Commit

Permalink
Move the RemoteFileArtifactValue methods dealing with expiration time…
Browse files Browse the repository at this point in the history 
… into the base class.

This makes it easier to change the FileArtifactValue class hierarchy.

PiperOrigin-RevId: 717919924
Change-Id: Ie44dc3f8eb15a0ab3e90e406bc02c8f23c938d1a
  • Loading branch information
@tjgq @copybara-github
tjgq authored and copybara-github committed Jan 21, 2025
1 parent 9507a1b commit 08c6d42
Show file tree
Hide file tree
Showing 12 changed files with 116 additions and 105 deletions.
102 changes: 50 additions & 52 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 @@ -127,19 +127,39 @@ public byte[] getValueFingerprint() {
return getDigest();
}

/** Returns whether the file contents exist remotely. */
public boolean isRemote() {
return false;
}

/** Returns the location index for remote files. For non-remote files, returns 0. */
public int getLocationIndex() {
return 0;
}

/**
* Index used to resolve remote files.
* Returns the time when the remote file contents expire. If the contents never expire, including
* when they're not remote, returns null.
*
* <p>0 indicates that no such information is available which can mean that it's either a local
* file, empty, or an omitted output.
* <p>The expiration time does not factor into equality, as it can be mutated by {@link
* #setExpirationTime}.
*/
public int getLocationIndex() {
return 0;
@Nullable
public Instant getExpirationTime() {
return null;
}

/** Returns whether the file contents exist remotely. */
public boolean isRemote() {
return false;
/**
* Sets the expiration time. If this metadata was originally constructed without an expiration
* time, does nothing.
*/
public void setExpirationTime(Instant newExpirationTime) {}

/**
* Returns whether the file contents are available (either locally, or remotely and not expired).
*/
public final boolean isAlive(Instant now) {
return getExpirationTime() == null || getExpirationTime().isAfter(now);
}

/**
Expand Down Expand Up @@ -566,10 +586,10 @@ public static RemoteFileArtifactValueWithMaterializationData createWithMateriali
byte[] digest,
long size,
int locationIndex,
long expireAtEpochMilli,
@Nullable Instant expirationTime,
@Nullable PathFragment materializationExecPath) {
return new RemoteFileArtifactValueWithMaterializationData(
digest, size, locationIndex, materializationExecPath, expireAtEpochMilli);
digest, size, locationIndex, materializationExecPath, expirationTime);
}

/**
Expand All @@ -586,7 +606,7 @@ public static RemoteFileArtifactValue createFromExistingWithMaterializationPath(
metadata.getDigest(),
metadata.getSize(),
metadata.getLocationIndex(),
metadata.getExpireAtEpochMilli(),
metadata.getExpirationTime(),
metadata.getMaterializationExecPath().orElse(materializationExecPath));
}

Expand Down Expand Up @@ -635,26 +655,6 @@ public final int getLocationIndex() {
return locationIndex;
}

/**
* Returns the time when the remote file expires in milliseconds since epoch. A negative value
* means the remote is not known to expire.
*
* <p>Expiration time does not contribute to equality of remote files.
*/
public long getExpireAtEpochMilli() {
return -1;
}

/**
* Extends the expiration time for this metadata. If it was constructed without known expiration
* time (i.e. expireAtEpochMilli < 0), this extension does nothing.
*/
public void extendExpireAtEpochMilli(long expireAtEpochMilli) {}

public boolean isAlive(Instant now) {
return true;
}

@Override
public final boolean wasModifiedSinceDigest(Path path) {
return false;
Expand Down Expand Up @@ -682,32 +682,38 @@ public String toString() {
public static final class RemoteFileArtifactValueWithMaterializationData
extends RemoteFileArtifactValue {
@Nullable private final PathFragment materializationExecPath;
private long expireAtEpochMilli;
private long expirationTime;
@Nullable private FileContentsProxy proxy;

private RemoteFileArtifactValueWithMaterializationData(
byte[] digest,
long size,
int locationIndex,
PathFragment materializationExecPath,
long expireAtEpochMilli) {
@Nullable PathFragment materializationExecPath,
@Nullable Instant expirationTime) {
super(digest, size, locationIndex);
this.materializationExecPath = materializationExecPath;
this.expireAtEpochMilli = expireAtEpochMilli;
this.expirationTime = toEpochMilli(expirationTime);
}

private static long toEpochMilli(@Nullable Instant expirationTime) {
return expirationTime != null ? expirationTime.toEpochMilli() : -1;
}

@Nullable
private static Instant fromEpochMilli(long expirationTime) {
return expirationTime >= 0 ? Instant.ofEpochMilli(expirationTime) : null;
}

@Override
public long getExpireAtEpochMilli() {
return expireAtEpochMilli;
@Nullable
public Instant getExpirationTime() {
return fromEpochMilli(expirationTime);
}

@Override
public void extendExpireAtEpochMilli(long expireAtEpochMilli) {
if (expireAtEpochMilli < 0) {
return;
}
checkState(expireAtEpochMilli > this.expireAtEpochMilli);
this.expireAtEpochMilli = expireAtEpochMilli;
public void setExpirationTime(Instant expirationTime) {
this.expirationTime = toEpochMilli(expirationTime);
}

/**
Expand Down Expand Up @@ -738,14 +744,6 @@ public boolean canSetContentsProxy() {
return true;
}

@Override
public boolean isAlive(Instant now) {
if (expireAtEpochMilli < 0) {
return true;
}
return now.toEpochMilli() < expireAtEpochMilli;
}

@Override
public Optional<PathFragment> getMaterializationExecPath() {
return Optional.ofNullable(materializationExecPath);
Expand Down Expand Up @@ -779,7 +777,7 @@ public String toString() {
.add("size", getSize())
.add("locationIndex", getLocationIndex())
.add("materializationExecPath", materializationExecPath)
.add("expireAtEpochMilli", expireAtEpochMilli)
.add("expirationTime", fromEpochMilli(expirationTime))
.add("proxy", proxy)
.toString();
}
Expand Down
13 changes: 9 additions & 4 deletions 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 @@ -506,7 +506,8 @@ private static void encodeRemoteMetadata(

VarInt.putVarInt(value.getLocationIndex(), sink);

VarInt.putVarLong(value.getExpireAtEpochMilli(), sink);
VarInt.putVarLong(
value.getExpirationTime() != null ? value.getExpirationTime().toEpochMilli() : -1, sink);

Optional<PathFragment> materializationExecPath = value.getMaterializationExecPath();
if (materializationExecPath.isPresent()) {
Expand All @@ -532,7 +533,7 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(

int locationIndex = VarInt.getVarInt(source);

long expireAtEpochMilli = VarInt.getVarLong(source);
long expirationTimeEpochMilli = VarInt.getVarLong(source);

PathFragment materializationExecPath = null;
int numMaterializationExecPath = VarInt.getVarInt(source);
Expand All @@ -544,12 +545,16 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(
PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)));
}

if (expireAtEpochMilli < 0 && materializationExecPath == null) {
if (expirationTimeEpochMilli < 0 && materializationExecPath == null) {
return RemoteFileArtifactValue.create(digest, size, locationIndex);
}

return RemoteFileArtifactValue.createWithMaterializationData(
digest, size, locationIndex, expireAtEpochMilli, materializationExecPath);
digest,
size,
locationIndex,
Instant.ofEpochMilli(expirationTimeEpochMilli),
materializationExecPath);
}

/**
Expand Down
5 changes: 3 additions & 2 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 @@ -59,6 +59,7 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.channels.SeekableByteChannel;
import java.time.Instant;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -299,7 +300,7 @@ public void updateContext(ActionExecutionMetadata action) {
this.action = action;
}

void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAtEpochMilli)
void injectRemoteFile(PathFragment path, byte[] digest, long size, Instant expirationTime)
throws IOException {
if (!isOutput(path)) {
return;
Expand All @@ -309,7 +310,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAt
digest,
size,
/* locationIndex= */ 1,
expireAtEpochMilli,
expirationTime,
/* materializationExecPath= */ null);
remoteOutputTree.injectFile(path, metadata);
}
Expand Down
6 changes: 3 additions & 3 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 @@ -1280,7 +1280,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
combinedCache, digestUtil, context, action.getRemotePathResolver());

// The expiration time for remote cache entries.
var expireAtEpochMilli = Instant.now().plus(remoteOptions.remoteCacheTtl).toEpochMilli();
var expirationTime = Instant.now().plus(remoteOptions.remoteCacheTtl);

ActionInput inMemoryOutput = null;
AtomicReference<ByteString> inMemoryOutputData = new AtomicReference<>(null);
Expand Down Expand Up @@ -1325,7 +1325,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
file.path().asFragment(),
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes(),
expireAtEpochMilli);
expirationTime);
}

if (isInMemoryOutputFile) {
Expand Down Expand Up @@ -1373,7 +1373,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
file.path().asFragment(),
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes(),
expireAtEpochMilli);
expirationTime);
}
}
}
Expand Down
23 changes: 11 additions & 12 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteLeaseExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.cache.ActionCache;
import com.google.devtools.build.lib.actions.cache.ActionCache.Entry;
import com.google.devtools.build.lib.actions.cache.ActionCache.Entry.SerializableTreeArtifactValue;
Expand Down Expand Up @@ -128,7 +127,7 @@ private void doExtendLeases() {
// issue one giant `FindMissingBlobs` call to avoid increasing memory footprint. Since
// this happens in the background, increased network calls are acceptable.
try (var silentCloseable1 = Profiler.instance().profile(action.describe())) {
extendLeaseForAction(action, remoteFiles, earliestExpiration.toEpochMilli());
extendLeaseForAction(action, remoteFiles, earliestExpiration);
}
}
}
Expand All @@ -152,7 +151,7 @@ private void doExtendLeases() {
}

private static boolean isRemoteMetadataWithTtl(FileArtifactValue metadata) {
return metadata.isRemote() && ((RemoteFileArtifactValue) metadata).getExpireAtEpochMilli() >= 0;
return metadata.isRemote() && metadata.getExpirationTime() != null;
}

private ImmutableList<Map.Entry<? extends Artifact, FileArtifactValue>> collectRemoteFiles(
Expand All @@ -179,7 +178,7 @@ private ImmutableList<Map.Entry<? extends Artifact, FileArtifactValue>> collectR
private void extendLeaseForAction(
Action action,
ImmutableList<Map.Entry<? extends Artifact, FileArtifactValue>> remoteFiles,
long expireAtEpochMilli)
Instant expirationTime)
throws IOException, InterruptedException {
ImmutableSet<Digest> missingDigests;
try (var silentCloseable = Profiler.instance().profile("findMissingDigests")) {
Expand All @@ -196,15 +195,15 @@ private void extendLeaseForAction(
var token = getActionCacheToken(action);
for (var remoteFile : remoteFiles) {
var artifact = remoteFile.getKey();
var metadata = (RemoteFileArtifactValue) remoteFile.getValue();
var metadata = remoteFile.getValue();
// Only extend the lease for the remote output if it is still alive remotely.
if (!missingDigests.contains(buildDigest(metadata))) {
metadata.extendExpireAtEpochMilli(expireAtEpochMilli);
metadata.setExpirationTime(expirationTime);
if (token != null) {
if (artifact instanceof TreeFileArtifact treeFileArtifact) {
token.extendOutputTreeFile(treeFileArtifact, expireAtEpochMilli);
token.extendOutputTreeFile(treeFileArtifact, expirationTime);
} else {
token.extendOutputFile(artifact, expireAtEpochMilli);
token.extendOutputFile(artifact, expirationTime);
}
}
}
Expand Down Expand Up @@ -260,20 +259,20 @@ private static class ActionCacheToken {
this.entry = entry;
}

void extendOutputFile(Artifact artifact, long expireAtEpochMilli) {
void extendOutputFile(Artifact artifact, Instant expirationTime) {
var metadata = entry.getOutputFile(artifact);
if (metadata != null) {
metadata.extendExpireAtEpochMilli(expireAtEpochMilli);
metadata.setExpirationTime(expirationTime);
dirty = true;
}
}

void extendOutputTreeFile(TreeFileArtifact treeFile, long expireAtEpochMilli) {
void extendOutputTreeFile(TreeFileArtifact treeFile, Instant expirationTime) {
SerializableTreeArtifactValue treeMetadata = entry.getOutputTree(treeFile.getParent());
if (treeMetadata != null) {
var metadata = treeMetadata.childValues().get(treeFile.getTreeRelativePathString());
if (metadata != null) {
metadata.extendExpireAtEpochMilli(expireAtEpochMilli);
metadata.setExpirationTime(expirationTime);
dirty = true;
}
}
Expand Down
18 changes: 12 additions & 6 deletions src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,14 +466,16 @@ private RemoteFileArtifactValue createRemoteFileMetadata(
String content, @Nullable PathFragment materializationExecPath) {
byte[] bytes = content.getBytes(UTF_8);
return RemoteFileArtifactValue.createWithMaterializationData(
digest(bytes), bytes.length, 1, /* expireAtEpochMilli= */ -1, materializationExecPath);
digest(bytes), bytes.length, 1, /* expirationTime= */ null, materializationExecPath);
}

private RemoteFileArtifactValue createRemoteFileMetadata(
String content, long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) {
String content,
@Nullable Instant expirationTime,
@Nullable PathFragment materializationExecPath) {
byte[] bytes = content.getBytes(UTF_8);
return RemoteFileArtifactValue.createWithMaterializationData(
digest(bytes), bytes.length, 1, expireAtEpochMilli, materializationExecPath);
digest(bytes), bytes.length, 1, expirationTime, materializationExecPath);
}

private static TreeArtifactValue createTreeMetadata(
Expand Down Expand Up @@ -609,7 +611,9 @@ public void saveOutputMetadata_remoteFileExpired_remoteFileMetadataNotLoaded() t
new InjectOutputFileMetadataAction(
output,
createRemoteFileMetadata(
content, /* expireAtEpochMilli= */ 0, /* materializationExecPath= */ null));
content,
/* expirationTime= */ Instant.ofEpochMilli(1),
/* materializationExecPath= */ null));
FakeInputMetadataHandler metadataHandler = new FakeInputMetadataHandler();

runAction(action);
Expand Down Expand Up @@ -1190,7 +1194,9 @@ public void saveOutputMetadata_treeFileExpired_treeMetadataNotLoaded() throws Ex
"file1", createRemoteFileMetadata("content1"),
"file2",
createRemoteFileMetadata(
"content2", /* expireAtEpochMilli= */ 0, /* materializationExecPath= */ null));
"content2",
/* expirationTime= */ Instant.ofEpochMilli(1),
/* materializationExecPath= */ null));
Action action =
new InjectOutputTreeMetadataAction(
output,
Expand Down Expand Up @@ -1240,7 +1246,7 @@ public void saveOutputMetadata_archivedRepresentationExpired_treeMetadataNotLoad
/* archivedArtifactValue= */ Optional.of(
createRemoteFileMetadata(
"archived",
/* expireAtEpochMilli= */ 0,
/* expirationTime= */ Instant.ofEpochMilli(1),
/* materializationExecPath= */ null)),
/* materializationExecPath= */ Optional.empty()));
FakeInputMetadataHandler metadataHandler = new FakeInputMetadataHandler();
Expand Down
Loading

0 comments on commit 08c6d42

Please sign in to comment.