Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Store TTL in RemoteFileArtifactValue #17639

Closed
wants to merge 2 commits into from
Closed

Store TTL in RemoteFileArtifactValue #17639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61711f1
Store TTL in RemoteFileArtifactValue.
coeuvre Mar 1, 2023
de37b24
Add more tests
coeuvre Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand Down Expand Up @@ -317,6 +318,7 @@ private CachedOutputMetadata(

private static CachedOutputMetadata loadCachedOutputMetadata(
Action action, ActionCache.Entry entry, MetadataHandler metadataHandler) {
Instant now = Instant.now();
ImmutableMap.Builder<Artifact, RemoteFileArtifactValue> remoteFileMetadata =
ImmutableMap.builder();
ImmutableMap.Builder<SpecialArtifact, TreeArtifactValue> mergedTreeMetadata =
Expand All @@ -330,6 +332,17 @@ private static CachedOutputMetadata loadCachedOutputMetadata(
continue;
}

// If any child is not alive, discard the entire tree
tjgq marked this conversation as resolved.
Show resolved Hide resolved
if (cachedTreeMetadata.childValues().values().stream()
.anyMatch(metadata -> !metadata.isAlive(now))) {
continue;
}

if (cachedTreeMetadata.archivedFileValue().isPresent()
&& !cachedTreeMetadata.archivedFileValue().get().isAlive(now)) {
continue;
}

TreeArtifactValue localTreeMetadata;
try {
localTreeMetadata = metadataHandler.getTreeArtifactValue(parent);
Expand Down Expand Up @@ -377,7 +390,7 @@ private static CachedOutputMetadata loadCachedOutputMetadata(
mergedTreeMetadata.put(parent, merged.build());
} else {
RemoteFileArtifactValue cachedMetadata = entry.getOutputFile(artifact);
if (cachedMetadata == null) {
if (cachedMetadata == null || !cachedMetadata.isAlive(now)) {
continue;
}

Expand Down
40 changes: 33 additions & 7 deletions src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.skyframe.SkyValue;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.time.Instant;
import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;
Expand Down Expand Up @@ -542,27 +543,34 @@ public static class RemoteFileArtifactValue extends FileArtifactValue {
protected final byte[] digest;
protected final long size;
protected final int locationIndex;
// The time when the remote file expires in milliseconds since epoch. negative value means the
// remote file will never expire. This field doesn't contribute to the equality of the metadata.
protected final long expireAtEpochMilli;

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

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

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

@Override
Expand Down Expand Up @@ -613,6 +621,18 @@ public int getLocationIndex() {
return locationIndex;
}

public long getExpireAtEpochMilli() {
return expireAtEpochMilli;
}

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

return now.toEpochMilli() < expireAtEpochMilli;
}

@Override
public boolean wasModifiedSinceDigest(Path path) {
return false;
Expand All @@ -629,6 +649,7 @@ public String toString() {
.add("digest", bytesToString(digest))
.add("size", size)
.add("locationIndex", locationIndex)
.add("expireAtEpochMilli", expireAtEpochMilli)
.toString();
}
}
Expand All @@ -644,8 +665,12 @@ public static final class RemoteFileArtifactValueWithMaterializationPath
private final PathFragment materializationExecPath;

private RemoteFileArtifactValueWithMaterializationPath(
byte[] digest, long size, int locationIndex, PathFragment materializationExecPath) {
super(digest, size, locationIndex);
byte[] digest,
long size,
int locationIndex,
long expireAtEpochMilli,
PathFragment materializationExecPath) {
super(digest, size, locationIndex, expireAtEpochMilli);
this.materializationExecPath = Preconditions.checkNotNull(materializationExecPath);
}

Expand Down Expand Up @@ -679,6 +704,7 @@ public String toString() {
.add("digest", bytesToString(digest))
.add("size", size)
.add("locationIndex", locationIndex)
.add("expireAtEpochMilli", expireAtEpochMilli)
.add("materializationExecPath", materializationExecPath)
.toString();
}
Expand Down
17 changes: 11 additions & 6 deletions src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public class CompactPersistentActionCache implements ActionCache {

private static final int NO_INPUT_DISCOVERY_COUNT = -1;

private static final int VERSION = 15;
private static final int VERSION = 16;

private static final class ActionMap extends PersistentMap<Integer, byte[]> {
private final Clock clock;
Expand Down Expand Up @@ -466,6 +466,8 @@ private static void encodeRemoteMetadata(

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

VarInt.putVarLong(value.getExpireAtEpochMilli(), sink);

Optional<PathFragment> materializationExecPath = value.getMaterializationExecPath();
if (materializationExecPath.isPresent()) {
VarInt.putVarInt(1, sink);
Expand All @@ -476,10 +478,11 @@ private static void encodeRemoteMetadata(
}

private static final int MAX_REMOTE_METADATA_SIZE =
DigestUtils.ESTIMATED_SIZE
+ VarInt.MAX_VARLONG_SIZE
+ VarInt.MAX_VARINT_SIZE
+ VarInt.MAX_VARINT_SIZE;
DigestUtils.ESTIMATED_SIZE // digest
+ VarInt.MAX_VARLONG_SIZE // size
+ VarInt.MAX_VARINT_SIZE // locationIndex
+ VarInt.MAX_VARINT_SIZE // expireAtEpochMilli
+ VarInt.MAX_VARINT_SIZE; // materializationExecPath

private static RemoteFileArtifactValue decodeRemoteMetadata(
StringIndexer indexer, ByteBuffer source) throws IOException {
Expand All @@ -489,6 +492,8 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(

int locationIndex = VarInt.getVarInt(source);

long expireAtEpochMilli = VarInt.getVarLong(source);

PathFragment materializationExecPath = null;
int numMaterializationExecPath = VarInt.getVarInt(source);
if (numMaterializationExecPath > 0) {
Expand All @@ -499,7 +504,7 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(
PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)));
}

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

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

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

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

Expand All @@ -214,8 +216,8 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu
}

private RemoteFileArtifactValue createRemoteMetadata(RemoteFileInfo remoteFile) {
return RemoteFileArtifactValue.create(
remoteFile.getFastDigest(), remoteFile.getSize(), /* locationIndex= */ 1);
return RemoteFileArtifactValue.create(remoteFile.getFastDigest(),
remoteFile.getSize(), /* locationIndex= */ 1, remoteFile.getExpireAtEpochMilli());
}

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

void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOException {
void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAtEpochMilli)
throws IOException {
createDirectoryAndParents(path.getParentDirectory());
InMemoryContentInfo node = getOrCreateWritableInode(path);
// If a node was already existed and is not a remote file node (i.e. directory or symlink node
Expand All @@ -753,7 +756,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOExce
}

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

@Nullable
Expand All @@ -771,13 +774,16 @@ static class RemoteFileInfo extends FileInfo {
private byte[] digest;
private long size;

private long expireAtEpochMilli;

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

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

@Override
Expand All @@ -804,5 +810,9 @@ public byte[] getFastDigest() {
public long getSize() {
return size;
}

public long getExpireAtEpochMilli() {
return expireAtEpochMilli;
}
}
}
12 changes: 8 additions & 4 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@
import io.reactivex.rxjava3.disposables.Disposable;
import io.reactivex.rxjava3.schedulers.Schedulers;
import java.io.IOException;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -806,7 +807,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 @@ -825,15 +826,17 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
remoteActionFileSystem.injectRemoteFile(
file.path().asFragment(),
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes());
file.digest().getSizeBytes(),
expireAtEpochMilli);
}
}

for (FileMetadata file : metadata.files()) {
remoteActionFileSystem.injectRemoteFile(
file.path().asFragment(),
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes());
file.digest().getSizeBytes(),
expireAtEpochMilli);
}
}

Expand Down Expand Up @@ -1162,7 +1165,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
}
}

injectRemoteArtifacts(action, metadata);
var expireAtEpochMilli = Instant.now().plus(remoteOptions.remoteCacheTtl).toEpochMilli();
injectRemoteArtifacts(action, metadata, expireAtEpochMilli);

try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) {
if (inMemoryOutput != null) {
Expand Down
39 changes: 39 additions & 0 deletions src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,16 @@
// limitations under the License.
package com.google.devtools.build.lib.remote.options;

import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.Converters;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParsingException;
import java.time.Duration;
import java.util.List;
import java.util.regex.Pattern;

/** Options for remote execution and distributed caching that shared between Bazel and Blaze. */
public class CommonRemoteOptions extends OptionsBase {
Expand All @@ -33,4 +38,38 @@ public class CommonRemoteOptions extends OptionsBase {
+ " the client to request certain artifacts that might be needed locally (e.g. IDE"
+ " support)")
public List<String> remoteDownloadRegex;

@Option(
name = "experimental_remote_cache_ttl",
defaultValue = "3h",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we go with a larger default, say 8h or even 24h? I'm mostly worried about this throwing off long-running benchmarks.

We should also add this to the relnotes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Higher value by default means higher space requirement on remote cas. I picked 3h randomly because it's on par with the default value for --max_idle_secs.

The TTL only affects incremental builds. If, for example, a long-running invocation lasts more than 3h, nothing goes wrong during this invocation. It's just for next incremental build, all remote metadata are discarded. Also, the plan is to have a background thread refresh the lease for remote metadata. So for long-running invocations, it doesn't matter what the value of --experimental_remote_cache_ttl is set to. The frequency of the refresh is based on --experimental_remote_cache_ttl, though.

That being said, I am open to give it a higher default value.

@brentleyjones, you commented on this flag in the prototype, do you have any suggestions here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it being as high as 24h, because I know there are some remote cache instances that can't reach that level of guarantee. And like you said, this doesn't impact long-running builds, only incremental builds. I'm good with 3h, but I don't think it should be larger than 8h.

documentationCategory = OptionDocumentationCategory.REMOTE,
effectTags = {OptionEffectTag.EXECUTION},
converter = RemoteDurationConverter.class,
help =
"The guaranteed minimal TTL of blobs in the remote cache after their digests are recently"
+ " referenced e.g. by an ActionResult or FindMissingBlobs. Bazel does several"
+ " optimizations based on the blobs' TTL e.g. doesn't repeatedly call"
+ " GetActionResult in an incremental build. The value should be set slightly less"
+ " than the real TTL since there is a gap between when the server returns the"
+ " digests and when Bazel receives them.")
public Duration remoteCacheTtl;

/** Returns the specified duration. Assumes seconds if unitless. */
public static class RemoteDurationConverter extends Converter.Contextless<Duration> {

private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$");

@Override
public Duration convert(String input) throws OptionsParsingException {
if (UNITLESS_REGEX.matcher(input).matches()) {
input += "s";
}
return new Converters.DurationConverter().convert(input, /* conversionContext= */ null);
}

@Override
public String getTypeDescription() {
return "An immutable length of time.";
}
}
}
Loading