Skip to content

Commit

Permalink
Store TTL in RemoteFileArtifactValue
Browse files Browse the repository at this point in the history
When building without the bytes, Bazel stores `RemoteFileArtifactValue` in skyframe (inmemory) and in local action cache which represents a file that is stored remotely. Bazel assumes that the remote file will never expire which is wrong. In practice, remote cache often evict files due to space constraint, and when it happens, the builds could fail.

This PR introduces flag `--experimental_remote_cache_ttl` which tells Bazel at least how long the remote cache could store a file after returning a reference of it to Bazel. Bazel calculates the TTL of the file and store it in the `RemoteFileArtifactValue`. In an incremental build, Bazel will discard the `RemoteFileArtifactValue` and rerun the generating actions if it finds out that the `RemoteFileArtifactValue` is expired. The new field `expireAtEpochMilli` replaces `actionId` (deleted by f62a8b9), so there shouldn't be memory regression.

There are two places Bazel checks the TTL:
1. If the skyframe has in-memory state about previous builds (e.g. incremental builds), the `SkyValue`s are marked as dirty if the `RemoteFileArtifactValue` is expired.
2. When checking local action cache, if the `RemoteFileArtifactValue` is expired, the cache entry is ignored.

So that the generating actions can be re-executed.

Part of bazelbuild#16660.

Closes bazelbuild#17639.

RELNOTES: Add flag `--experimental_remote_cache_ttl` and set the default value to 3 hours.
PiperOrigin-RevId: 513819724
Change-Id: I9c9813621d04d5b1b94312be39384962feae2f7b
  • Loading branch information
coeuvre authored and fweikert committed May 25, 2023
1 parent 3f7dfcb commit bd01094
Show file tree
Hide file tree
Showing 23 changed files with 635 additions and 120 deletions.
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
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
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
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,8 @@ 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
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 @@ -215,7 +217,10 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu

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

@Override
Expand Down Expand Up @@ -743,7 +748,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 +759,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 +777,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 +813,9 @@ public byte[] getFastDigest() {
public long getSize() {
return size;
}

public long getExpireAtEpochMilli() {
return expireAtEpochMilli;
}
}
}
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,8 @@ 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 +827,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 +1166,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
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",
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

0 comments on commit bd01094

Please sign in to comment.