Skip to content

Commit

Permalink
[6.2.0] Remove actionId from RemoteFileArtifactValue. (#17724)
Browse files Browse the repository at this point in the history 
actionId was added by #11236 to track download requests for input prefetches.

However, according to the REAPI spec, action_id is

> An identifier that ties multiple requests to the same action. For example, multiple requests to the CAS, Action Cache, and Execution API are used in order to compile foo.cc.

The input prefetches are requested by local actions that depend on outputs of remote executed action (which is identified by the action_id). These requests are not part of generating action hence shouldn't be bound with the action_id.

We could include ActionExecutionMetadata of the local action for the requests for the tracking purpose, but that desires another CL.

PiperOrigin-RevId: 510430853
Change-Id: I8d64a4f7033320a394e02a2b11498f09a3d15310

Co-authored-by: Googler <[email protected]>
  • Loading branch information
@tjgq @coeuvre
tjgq and coeuvre authored Mar 10, 2023
1 parent 94c519b commit 60749d5
Show file tree
Hide file tree
Showing 13 changed files with 145 additions and 185 deletions.
51 changes: 10 additions & 41 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 @@ -109,15 +109,6 @@ public int getLocationIndex() {
return 0;
}

/**
* Remote action source identifier for the file.
*
* <p>"" indicates that a remote action output was not the source of this artifact.
*/
public String getActionId() {
return "";
}

/** Returns {@code true} if the file only exists remotely. */
public boolean isRemote() {
return false;
Expand Down Expand Up @@ -550,35 +541,27 @@ public static class RemoteFileArtifactValue extends FileArtifactValue {
protected final byte[] digest;
protected final long size;
protected final int locationIndex;
protected final String actionId;

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

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

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

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

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

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

@Override
Expand All @@ -619,11 +601,6 @@ public long getSize() {
return size;
}

@Override
public String getActionId() {
return actionId;
}

@Override
public long getModifiedTime() {
throw new UnsupportedOperationException(
Expand Down Expand Up @@ -651,7 +628,6 @@ public String toString() {
.add("digest", bytesToString(digest))
.add("size", size)
.add("locationIndex", locationIndex)
.add("actionId", actionId)
.toString();
}
}
Expand All @@ -667,12 +643,8 @@ public static final class RemoteFileArtifactValueWithMaterializationPath
private final PathFragment materializationExecPath;

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

Expand All @@ -692,14 +664,12 @@ public boolean equals(Object o) {
return Arrays.equals(digest, that.digest)
&& size == that.size
&& locationIndex == that.locationIndex
&& Objects.equals(actionId, that.actionId)
&& Objects.equals(materializationExecPath, that.materializationExecPath);
}

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

@Override
Expand All @@ -708,7 +678,6 @@ public String toString() {
.add("digest", bytesToString(digest))
.add("size", size)
.add("locationIndex", locationIndex)
.add("actionId", actionId)
.add("materializationExecPath", materializationExecPath)
.toString();
}
Expand Down
9 changes: 2 additions & 7 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 @@ -76,7 +76,7 @@ public class CompactPersistentActionCache implements ActionCache {

private static final int NO_INPUT_DISCOVERY_COUNT = -1;

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

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

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

VarInt.putVarInt(indexer.getOrCreateIndex(value.getActionId()), sink);

Optional<PathFragment> materializationExecPath = value.getMaterializationExecPath();
if (materializationExecPath.isPresent()) {
VarInt.putVarInt(1, sink);
Expand All @@ -491,8 +489,6 @@ private static RemoteFileArtifactValue decodeRemoteMetadata(

int locationIndex = VarInt.getVarInt(source);

String actionId = getStringForIndex(indexer, VarInt.getVarInt(source));

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

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

/** @return action data encoded as a byte[] array. */
Expand Down
24 changes: 6 additions & 18 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 @@ -115,12 +115,11 @@ public void updateContext(MetadataInjector metadataInjector) {
this.metadataInjector = metadataInjector;
}

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

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

Expand All @@ -217,10 +215,7 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu

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

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

void injectRemoteFile(PathFragment path, byte[] digest, long size, String actionId)
throws IOException {
void injectRemoteFile(PathFragment path, byte[] digest, long size) 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 @@ -759,7 +753,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size, String action
}

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

@Nullable
Expand All @@ -776,16 +770,14 @@ static class RemoteFileInfo extends FileInfo {

private byte[] digest;
private long size;
private String actionId;

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

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

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

public String getActionId() {
return actionId;
}
}
}
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ protected ListenableFuture<Void> doDownloadFile(
throws IOException {
checkArgument(metadata.isRemote(), "Cannot download file that is not a remote file.");
RequestMetadata requestMetadata =
TracingMetadataUtils.buildMetadata(buildRequestId, commandId, metadata.getActionId(), null);
TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "prefetcher", null);
RemoteActionExecutionContext context = RemoteActionExecutionContext.create(requestMetadata);

Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
Expand Down
7 changes: 2 additions & 5 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 @@ -812,7 +812,6 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
FileSystem actionFileSystem = action.getSpawnExecutionContext().getActionFileSystem();
checkState(actionFileSystem instanceof RemoteActionFileSystem);

RemoteActionExecutionContext context = action.getRemoteActionExecutionContext();
RemoteActionFileSystem remoteActionFileSystem = (RemoteActionFileSystem) actionFileSystem;

for (Map.Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
Expand All @@ -827,17 +826,15 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
remoteActionFileSystem.injectRemoteFile(
file.path().asFragment(),
DigestUtil.toBinaryDigest(file.digest()),
file.digest().getSizeBytes(),
context.getRequestMetadata().getActionId());
file.digest().getSizeBytes());
}
}

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

Expand Down
3 changes: 1 addition & 2 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 @@ -463,8 +463,7 @@ private RemoteFileArtifactValue createRemoteFileMetadata(String content) {
private RemoteFileArtifactValue createRemoteFileMetadata(
String content, @Nullable PathFragment materializationExecPath) {
byte[] bytes = content.getBytes(UTF_8);
return RemoteFileArtifactValue.create(
digest(bytes), bytes.length, 1, "action-id", materializationExecPath);
return RemoteFileArtifactValue.create(digest(bytes), bytes.length, 1, materializationExecPath);
}

private static TreeArtifactValue createTreeMetadata(
Expand Down
3 changes: 1 addition & 2 deletions ...st/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,7 @@ private RemoteFileArtifactValue createRemoteMetadata(
.getHashFunction()
.hashBytes(bytes)
.asBytes();
return RemoteFileArtifactValue.create(
digest, bytes.length, 1, "action-id", materializationExecPath);
return RemoteFileArtifactValue.create(digest, bytes.length, 1, materializationExecPath);
}

private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) {
Expand Down
3 changes: 1 addition & 2 deletions src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ protected Artifact createRemoteArtifact(
hashCode.asBytes(),
contentsBytes.length,
/* locationIndex= */ 1,
"action-id",
materializationExecPath);
metadata.put(a, f);
if (cas != null) {
Expand Down Expand Up @@ -136,7 +135,7 @@ protected Pair<SpecialArtifact, ImmutableList<TreeFileArtifact>> createRemoteTre
TreeFileArtifact.createTreeOutput(parent, PathFragment.create(entry.getKey()));
RemoteFileArtifactValue childValue =
RemoteFileArtifactValue.create(
hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1, "action-id");
hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1);
treeBuilder.putChild(child, childValue);
metadata.put(child, childValue);
cas.put(hashCode, contentsBytes);
Expand Down
4 changes: 1 addition & 3 deletions ...t/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ private Artifact createRemoteArtifact(
byte[] b = contents.getBytes(StandardCharsets.UTF_8);
HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash());
FileArtifactValue f =
RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id");
RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1);
inputs.putWithNoDepOwner(a, f);
return a;
}
Expand Down Expand Up @@ -513,8 +513,6 @@ public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(

private static class AllMissingDigestsFinder implements MissingDigestsFinder {

public static final AllMissingDigestsFinder INSTANCE = new AllMissingDigestsFinder();

@Override
public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
RemoteActionExecutionContext context, Iterable<Digest> digests) {
Expand Down
7 changes: 3 additions & 4 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ protected void injectRemoteFile(FileSystem actionFs, PathFragment path, String c
byte[] contentBytes = content.getBytes(StandardCharsets.UTF_8);
HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentBytes);
((RemoteActionFileSystem) actionFs)
.injectRemoteFile(path, hashCode.asBytes(), contentBytes.length, "action-id");
.injectRemoteFile(path, hashCode.asBytes(), contentBytes.length);
}

@Override
Expand All @@ -342,7 +342,7 @@ private Artifact createRemoteArtifact(
byte[] b = contents.getBytes(StandardCharsets.UTF_8);
HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b);
RemoteFileArtifactValue f =
RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id");
RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1);
inputs.putWithNoDepOwner(a, f);
return a;
}
Expand All @@ -364,8 +364,7 @@ private TreeArtifactValue createRemoteTreeArtifactValue(
byte[] b = entry.getValue().getBytes(StandardCharsets.UTF_8);
HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b);
RemoteFileArtifactValue childMeta =
RemoteFileArtifactValue.create(
h.asBytes(), b.length, /* locationIndex= */ 0, "action-id");
RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 0);
builder.putChild(child, childMeta);
}
return builder.build();
Expand Down
Loading

0 comments on commit 60749d5

Please sign in to comment.