Fix FrontierNodeVersion multiple instantiation and add logging.
getSkyValueVersion calls were still creating multiple FrontierNodeVersion
objects, and I realized that it was missing double-checked locking in the
synchronized block.
Also remove the call to serialize a simple string repr of the active directories -- we can pass the actual byte array instead.
Also added a log line for the components of the skyvalue version for debugging.
```
241024 09:20:21.005:I 51 [com.google.devtools.build.lib.buildtool.BuildTool.buildTargets:223] Build identifier: 2d2c2b5c-1930-4d5f-86a2-759b3e3814f7
241024 09:21:26.650:I 2486 [com.google.devtools.build.lib.buildtool.BuildTool$RemoteAnalysisCachingDependenciesProviderImpl.getSkyValueVersion:1182] Remote analysis caching SkyValue version: FrontierNodeVersion{topLevelConfig=-75950329, directoryMatcher=-1987693526, blazeInstall=-542573734, precomputed=688156809}
```
PiperOrigin-RevId: 689369335
Change-Id: I0931bc6662d83b5e5a107aa541715b509be67857
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index 55ac41e..53333e1 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -1157,17 +1157,31 @@
return activeDirectoriesMatcher.includes(pkg.getPackageFragment());
}
- private FrontierNodeVersion frontierNodeVersionSingleton = null;
+ private volatile FrontierNodeVersion frontierNodeVersionSingleton = null;
+ /**
+ * Returns a byte array to uniquely version SkyValues for serialization.
+ *
+ * <p>This should only be caalled when Bazel has determined values for all version components
+ * for instantiating a {@link FrontierNodeVersion}.
+ *
+ * <p>This could be in the constructor if we know about the {@code topLevelConfig} component at
+ * initialization, but it is created much later during the deserialization pass.
+ */
@Override
- public FrontierNodeVersion getSkyValueVersion() throws SerializationException {
+ public FrontierNodeVersion getSkyValueVersion() {
if (frontierNodeVersionSingleton == null) {
synchronized (this) {
- frontierNodeVersionSingleton =
- new FrontierNodeVersion(
- topLevelConfig.checksum(),
- getObjectCodecs().serializeMemoized(activeDirectoriesMatcher.toString()),
- blazeInstallMD5);
+ // Avoid re-initializing the value for subsequent threads with double checked locking.
+ if (frontierNodeVersionSingleton == null) {
+ frontierNodeVersionSingleton =
+ new FrontierNodeVersion(
+ topLevelConfig.checksum(),
+ activeDirectoriesMatcher.toString(),
+ blazeInstallMD5);
+ logger.atInfo().log(
+ "Remote analysis caching SkyValue version: %s", frontierNodeVersionSingleton);
+ }
}
}
return frontierNodeVersionSingleton;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java
index fff88cd..d1241f8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java
@@ -20,6 +20,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.MoreObjects;
import com.google.common.hash.HashCode;
import com.google.common.primitives.Bytes;
import com.google.common.util.concurrent.Futures;
@@ -328,8 +329,7 @@
/** A tuple representing the version of a cached SkyValue in the frontier. */
public static final class FrontierNodeVersion {
public static final FrontierNodeVersion CONSTANT_FOR_TESTING =
- new FrontierNodeVersion(
- "123", ByteString.copyFrom(new byte[] {1, 2, 3}), HashCode.fromInt(42));
+ new FrontierNodeVersion("123", "string_for_testing", HashCode.fromInt(42));
private final byte[] topLevelConfigFingerprint;
private final byte[] directoryMatcherFingerprint;
private final byte[] blazeInstallMD5Fingerprint;
@@ -337,11 +337,11 @@
public FrontierNodeVersion(
String topLevelConfigChecksum,
- ByteString directoryMatcherFingerprint,
+ String directoryMatcherStringRepr,
HashCode blazeInstallMD5) {
// TODO: b/364831651 - add more fields like source and blaze versions.
this.topLevelConfigFingerprint = topLevelConfigChecksum.getBytes(UTF_8);
- this.directoryMatcherFingerprint = directoryMatcherFingerprint.toByteArray();
+ this.directoryMatcherFingerprint = directoryMatcherStringRepr.getBytes(UTF_8);
this.blazeInstallMD5Fingerprint = blazeInstallMD5.asBytes();
this.precomputedFingerprint =
Bytes.concat(
@@ -359,6 +359,16 @@
}
@Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("topLevelConfig", Arrays.hashCode(topLevelConfigFingerprint))
+ .add("directoryMatcher", Arrays.hashCode(directoryMatcherFingerprint))
+ .add("blazeInstall", Arrays.hashCode(blazeInstallMD5Fingerprint))
+ .add("precomputed", hashCode())
+ .toString();
+ }
+
+ @Override
public int hashCode() {
return Arrays.hashCode(precomputedFingerprint);
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java
index 7e78404..ed742f5 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java
@@ -167,7 +167,7 @@
var version =
new FrontierNodeVersion(
/* topLevelConfigChecksum= */ "42",
- /* directoryMatcherFingerprint= */ ByteString.copyFrom(new byte[] {1, 2, 3}),
+ /* directoryMatcherStringRepr= */ "some_string",
/* blazeInstallMD5= */ HashCode.fromInt(42));
uploadKeyValuePair(key, version, value, fingerprintValueService);
@@ -194,7 +194,7 @@
var version =
new FrontierNodeVersion(
/* topLevelConfigChecksum= */ "42",
- /* directoryMatcherFingerprint= */ ByteString.copyFrom(new byte[] {1, 2, 3}),
+ /* directoryMatcherStringRepr= */ "some_string",
/* blazeInstallMD5= */ HashCode.fromInt(42));
uploadKeyValuePair(key, version, value, fingerprintValueService);
@@ -208,7 +208,7 @@
/* stateProvider= */ this,
/* frontierNodeVersion= */ new FrontierNodeVersion(
/* topLevelConfigChecksum= */ "9000",
- /* directoryMatcherFingerprint= */ ByteString.copyFrom(new byte[] {7, 8, 9}),
+ /* directoryMatcherStringRepr= */ "another_string",
/* blazeInstallMD5= */ HashCode.fromInt(9000)));
assertThat(result).isSameInstanceAs(NO_CACHED_DATA);
@@ -546,10 +546,8 @@
@Test
public void frontierNodeVersions_areEqual_ifTupleComponentsAreEqual() {
- var first =
- new FrontierNodeVersion("foo", ByteString.copyFromUtf8("bar"), HashCode.fromInt(42));
- var second =
- new FrontierNodeVersion("foo", ByteString.copyFromUtf8("bar"), HashCode.fromInt(42));
+ var first = new FrontierNodeVersion("foo", "bar", HashCode.fromInt(42));
+ var second = new FrontierNodeVersion("foo", "bar", HashCode.fromInt(42));
assertThat(first.getPrecomputedFingerprint()).isEqualTo(second.getPrecomputedFingerprint());
assertThat(first).isEqualTo(second);
@@ -557,10 +555,8 @@
@Test
public void frontierNodeVersions_areNotEqual_ifTopLevelConfigChecksumIsDifferent() {
- var first =
- new FrontierNodeVersion("foo", ByteString.copyFromUtf8("bar"), HashCode.fromInt(42));
- var second =
- new FrontierNodeVersion("CHANGED", ByteString.copyFromUtf8("bar"), HashCode.fromInt(42));
+ var first = new FrontierNodeVersion("foo", "bar", HashCode.fromInt(42));
+ var second = new FrontierNodeVersion("CHANGED", "bar", HashCode.fromInt(42));
assertThat(first.getPrecomputedFingerprint()).isNotEqualTo(second.getPrecomputedFingerprint());
assertThat(first).isNotEqualTo(second);
@@ -568,10 +564,8 @@
@Test
public void frontierNodeVersions_areNotEqual_ifActiveDirectoriesAreDifferent() {
- var first =
- new FrontierNodeVersion("foo", ByteString.copyFromUtf8("bar"), HashCode.fromInt(42));
- var second =
- new FrontierNodeVersion("foo", ByteString.copyFromUtf8("CHANGED"), HashCode.fromInt(42));
+ var first = new FrontierNodeVersion("foo", "bar", HashCode.fromInt(42));
+ var second = new FrontierNodeVersion("foo", "CHANGED", HashCode.fromInt(42));
assertThat(first.getPrecomputedFingerprint()).isNotEqualTo(second.getPrecomputedFingerprint());
assertThat(first).isNotEqualTo(second);
@@ -579,10 +573,8 @@
@Test
public void frontierNodeVersions_areNotEqual_ifBlazeInstallMD5IsDifferent() {
- var first =
- new FrontierNodeVersion("foo", ByteString.copyFromUtf8("bar"), HashCode.fromInt(42));
- var second =
- new FrontierNodeVersion("foo", ByteString.copyFromUtf8("bar"), HashCode.fromInt(9000));
+ var first = new FrontierNodeVersion("foo", "bar", HashCode.fromInt(42));
+ var second = new FrontierNodeVersion("foo", "bar", HashCode.fromInt(9000));
assertThat(first.getPrecomputedFingerprint()).isNotEqualTo(second.getPrecomputedFingerprint());
assertThat(first).isNotEqualTo(second);
