Skip to content

Commit

Permalink
Switch RemoteFileArtifactValue subclassing to optimize for memory c…
Browse files Browse the repository at this point in the history
…ost.

Adding `long expireAtEpochMilli` added 8 bytes to every `RemoteFileArtifactValue`, even ones without an expiration time. In contrast, `PathFragment materializationExecPath` is just a pointer so only costs 4 bytes, and since without it the cost is rounded up to the next multiple of 8 anyway (28 -> 32), it is free to  add this field.

Conveniently, since the expiration doesn't factor into `hashCode`/`equals`, it's actually simpler to arrange the classes this way.

PiperOrigin-RevId: 514388630
Change-Id: I50a7e4e03b266f1a2dfad2fa38005c0c9a0cfa10
  • Loading branch information
justinhorvitz authored and copybara-github committed Mar 6, 2023
1 parent 7c235ff commit 28dc0f9
Showing 1 changed file with 59 additions and 77 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ public long getSize() {
}

@Override
public boolean wasModifiedSinceDigest(Path path) throws IOException {
public boolean wasModifiedSinceDigest(Path path) {
return false;
}

Expand Down Expand Up @@ -540,24 +540,26 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue o) {

/** Metadata for remotely stored files. */
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 final byte[] digest;
private final long size;
private final int locationIndex;
@Nullable private final PathFragment materializationExecPath;

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

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

public static RemoteFileArtifactValue create(
Expand All @@ -566,147 +568,127 @@ public static RemoteFileArtifactValue create(
int locationIndex,
long expireAtEpochMilli,
@Nullable PathFragment materializationExecPath) {
if (materializationExecPath != null) {
return new RemoteFileArtifactValueWithMaterializationPath(
digest, size, locationIndex, expireAtEpochMilli, materializationExecPath);
}
return new RemoteFileArtifactValue(digest, size, locationIndex, expireAtEpochMilli);
return expireAtEpochMilli < 0
? new RemoteFileArtifactValue(digest, size, locationIndex, materializationExecPath)
: new RemoteFileArtifactValueWithExpiration(
digest, size, locationIndex, materializationExecPath, expireAtEpochMilli);
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof RemoteFileArtifactValue)) {
return false;
}

RemoteFileArtifactValue that = (RemoteFileArtifactValue) o;
return Arrays.equals(digest, that.digest)
&& size == that.size
&& locationIndex == that.locationIndex;
&& locationIndex == that.locationIndex
&& Objects.equals(materializationExecPath, that.materializationExecPath);
}

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

@Override
public FileStateType getType() {
public final FileStateType getType() {
return FileStateType.REGULAR_FILE;
}

@Override
public byte[] getDigest() {
public final byte[] getDigest() {
return digest;
}

@Override
public FileContentsProxy getContentsProxy() {
public final FileContentsProxy getContentsProxy() {
throw new UnsupportedOperationException();
}

@Override
public long getSize() {
public final long getSize() {
return size;
}

@Override
public long getModifiedTime() {
public final long getModifiedTime() {
throw new UnsupportedOperationException(
"RemoteFileArtifactValue doesn't support getModifiedTime");
}

@Override
public int getLocationIndex() {
public final int getLocationIndex() {
return locationIndex;
}

@Override
public final Optional<PathFragment> getMaterializationExecPath() {
return Optional.ofNullable(materializationExecPath);
}

/**
* 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 expireAtEpochMilli;
return -1;
}

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

return now.toEpochMilli() < expireAtEpochMilli;
return true;
}

@Override
public boolean wasModifiedSinceDigest(Path path) {
public final boolean wasModifiedSinceDigest(Path path) {
return false;
}

@Override
public boolean isRemote() {
public final boolean isRemote() {
return true;
}

@Override
public String toString() {
public final String toString() {
return MoreObjects.toStringHelper(this)
.add("digest", bytesToString(digest))
.add("size", size)
.add("locationIndex", locationIndex)
.add("expireAtEpochMilli", expireAtEpochMilli)
.add("materializationExecPath", materializationExecPath)
.add("expireAtEpochMilli", getExpireAtEpochMilli())
.toString();
}
}

/**
* A remote artifact that should be materialized in the local filesystem as a symlink to another
* location.
*
* <p>See the documentation for {@link FileArtifactValue#getMaterializationExecPath}.
*/
public static final class RemoteFileArtifactValueWithMaterializationPath
extends RemoteFileArtifactValue {
private final PathFragment materializationExecPath;
/** A remote artifact that expires at a particular time. */
private static final class RemoteFileArtifactValueWithExpiration extends RemoteFileArtifactValue {
private final long expireAtEpochMilli;

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

@Override
public Optional<PathFragment> getMaterializationExecPath() {
return Optional.ofNullable(materializationExecPath);
}

@Override
public boolean equals(Object o) {
if (!(o instanceof RemoteFileArtifactValueWithMaterializationPath)) {
return false;
}

RemoteFileArtifactValueWithMaterializationPath that =
(RemoteFileArtifactValueWithMaterializationPath) o;
return Arrays.equals(digest, that.digest)
&& size == that.size
&& locationIndex == that.locationIndex
&& Objects.equals(materializationExecPath, that.materializationExecPath);
PathFragment materializationExecPath,
long expireAtEpochMilli) {
super(digest, size, locationIndex, materializationExecPath);
this.expireAtEpochMilli = expireAtEpochMilli;
}

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

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("digest", bytesToString(digest))
.add("size", size)
.add("locationIndex", locationIndex)
.add("expireAtEpochMilli", expireAtEpochMilli)
.add("materializationExecPath", materializationExecPath)
.toString();
public boolean isAlive(Instant now) {
return now.toEpochMilli() < expireAtEpochMilli;
}
}

Expand Down

0 comments on commit 28dc0f9

Please sign in to comment.