Precompute hash code in `AspectValueKey` and `AbstractSkyKey`.
All instances of these classes are interned.
PiperOrigin-RevId: 371412973
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
index 63f284d..1d73b1c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
@@ -82,40 +82,39 @@
@Nullable BuildConfiguration targetConfiguration,
Label starlarkFileLabel,
String starlarkExportName) {
- StarlarkAspectLoadingKey key =
- new StarlarkAspectLoadingKey(
- targetLabel,
- aspectConfiguration == null ? null : BuildConfigurationValue.key(aspectConfiguration),
- ConfiguredTargetKey.builder()
- .setLabel(targetLabel)
- .setConfiguration(targetConfiguration)
- .build(),
- starlarkFileLabel,
- starlarkExportName);
-
- return starlarkAspectKeyInterner.intern(key);
+ return StarlarkAspectLoadingKey.createInternal(
+ targetLabel,
+ aspectConfiguration == null ? null : BuildConfigurationValue.key(aspectConfiguration),
+ ConfiguredTargetKey.builder()
+ .setLabel(targetLabel)
+ .setConfiguration(targetConfiguration)
+ .build(),
+ starlarkFileLabel,
+ starlarkExportName);
}
// Specific subtypes of aspect keys.
- /** A base class for a key representing an aspect applied to a particular target. */
+ /** Represents an aspect applied to a particular target. */
@AutoCodec
- public static class AspectKey extends AspectValueKey {
+ public static final class AspectKey extends AspectValueKey {
+ private final ConfiguredTargetKey baseConfiguredTargetKey;
private final ImmutableList<AspectKey> baseKeys;
@Nullable private final BuildConfigurationValue.Key aspectConfigurationKey;
- private final ConfiguredTargetKey baseConfiguredTargetKey;
private final AspectDescriptor aspectDescriptor;
- private int hashCode;
+ private final int hashCode;
private AspectKey(
- @Nullable BuildConfigurationValue.Key aspectConfigurationKey,
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
- AspectDescriptor aspectDescriptor) {
+ AspectDescriptor aspectDescriptor,
+ @Nullable BuildConfigurationValue.Key aspectConfigurationKey,
+ int hashCode) {
this.baseKeys = baseKeys;
this.aspectConfigurationKey = aspectConfigurationKey;
this.baseConfiguredTargetKey = baseConfiguredTargetKey;
this.aspectDescriptor = aspectDescriptor;
+ this.hashCode = hashCode;
}
@AutoCodec.VisibleForSerialization
@@ -127,7 +126,12 @@
@Nullable BuildConfigurationValue.Key aspectConfigurationKey) {
return aspectKeyInterner.intern(
new AspectKey(
- aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor));
+ baseConfiguredTargetKey,
+ baseKeys,
+ aspectDescriptor,
+ aspectConfigurationKey,
+ Objects.hashCode(
+ baseConfiguredTargetKey, baseKeys, aspectDescriptor, aspectConfigurationKey)));
}
@Override
@@ -169,8 +173,8 @@
return String.format("%s of %s",
aspectDescriptor.getAspectClass().getName(), getLabel());
} else {
- return String.format("%s on top of %s",
- aspectDescriptor.getAspectClass().getName(), baseKeys.toString());
+ return String.format(
+ "%s on top of %s", aspectDescriptor.getAspectClass().getName(), baseKeys);
}
}
@@ -205,33 +209,7 @@
@Override
public int hashCode() {
- // We use the hash code caching strategy employed by java.lang.String. There are three subtle
- // things going on here:
- //
- // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet.
- // Yes, this means that if the hash code is really 0 then we will "recompute" it each time.
- // But this isn't a problem in practice since a hash code of 0 should be rare.
- //
- // (2) Since we have no synchronization, multiple threads can race here thinking there are the
- // first one to compute and cache the hash code.
- //
- // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one
- // thread may not be visible by another.
- //
- // All three of these issues are benign from a correctness perspective; in the end we have no
- // overhead from synchronization, at the cost of potentially computing the hash code more than
- // once.
- int h = hashCode;
- if (h == 0) {
- h = computeHashCode();
- hashCode = h;
- }
- return h;
- }
-
- private int computeHashCode() {
- return Objects.hashCode(
- baseKeys, aspectConfigurationKey, baseConfiguredTargetKey, aspectDescriptor);
+ return hashCode;
}
@Override
@@ -239,13 +217,12 @@
if (this == other) {
return true;
}
-
if (!(other instanceof AspectKey)) {
return false;
}
-
AspectKey that = (AspectKey) other;
- return Objects.equal(baseKeys, that.baseKeys)
+ return hashCode == that.hashCode
+ && Objects.equal(baseKeys, that.baseKeys)
&& Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
&& Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
&& Objects.equal(aspectDescriptor, that.aspectDescriptor);
@@ -256,10 +233,7 @@
return "null";
}
- String baseKeysString =
- baseKeys.isEmpty()
- ? ""
- : String.format(" (over %s)", baseKeys.toString());
+ String baseKeysString = baseKeys.isEmpty() ? "" : String.format(" (over %s)", baseKeys);
return String.format(
"%s with aspect %s%s",
getLabel(), aspectDescriptor.getAspectClass().getName(), baseKeysString);
@@ -296,26 +270,51 @@
}
/** The key for a Starlark aspect. */
- public static class StarlarkAspectLoadingKey extends AspectValueKey {
-
+ @AutoCodec
+ public static final class StarlarkAspectLoadingKey extends AspectValueKey {
private final Label targetLabel;
private final BuildConfigurationValue.Key aspectConfigurationKey;
private final ConfiguredTargetKey baseConfiguredTargetKey;
private final Label starlarkFileLabel;
private final String starlarkValueName;
- private int hashCode;
+ private final int hashCode;
+
+ @AutoCodec.Instantiator
+ @AutoCodec.VisibleForSerialization
+ static StarlarkAspectLoadingKey createInternal(
+ Label targetLabel,
+ BuildConfigurationValue.Key aspectConfigurationKey,
+ ConfiguredTargetKey baseConfiguredTargetKey,
+ Label starlarkFileLabel,
+ String starlarkValueName) {
+ return starlarkAspectKeyInterner.intern(
+ new StarlarkAspectLoadingKey(
+ targetLabel,
+ aspectConfigurationKey,
+ baseConfiguredTargetKey,
+ starlarkFileLabel,
+ starlarkValueName,
+ Objects.hashCode(
+ targetLabel,
+ aspectConfigurationKey,
+ baseConfiguredTargetKey,
+ starlarkFileLabel,
+ starlarkValueName)));
+ }
private StarlarkAspectLoadingKey(
Label targetLabel,
BuildConfigurationValue.Key aspectConfigurationKey,
ConfiguredTargetKey baseConfiguredTargetKey,
Label starlarkFileLabel,
- String starlarkFunctionName) {
+ String starlarkValueName,
+ int hashCode) {
this.targetLabel = targetLabel;
this.aspectConfigurationKey = aspectConfigurationKey;
this.baseConfiguredTargetKey = baseConfiguredTargetKey;
this.starlarkFileLabel = starlarkFileLabel;
- this.starlarkValueName = starlarkFunctionName;
+ this.starlarkValueName = starlarkValueName;
+ this.hashCode = hashCode;
}
@Override
@@ -361,37 +360,7 @@
@Override
public int hashCode() {
- // We use the hash code caching strategy employed by java.lang.String. There are three subtle
- // things going on here:
- //
- // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet.
- // Yes, this means that if the hash code is really 0 then we will "recompute" it each time.
- // But this isn't a problem in practice since a hash code of 0 should be rare.
- //
- // (2) Since we have no synchronization, multiple threads can race here thinking there are the
- // first one to compute and cache the hash code.
- //
- // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one
- // thread may not be visible by another.
- //
- // All three of these issues are benign from a correctness perspective; in the end we have no
- // overhead from synchronization, at the cost of potentially computing the hash code more than
- // once.
- int h = hashCode;
- if (h == 0) {
- h = computeHashCode();
- hashCode = h;
- }
- return h;
- }
-
- private int computeHashCode() {
- return Objects.hashCode(
- targetLabel,
- aspectConfigurationKey,
- baseConfiguredTargetKey,
- starlarkFileLabel,
- starlarkValueName);
+ return hashCode;
}
@Override
@@ -399,12 +368,12 @@
if (o == this) {
return true;
}
-
if (!(o instanceof StarlarkAspectLoadingKey)) {
return false;
}
StarlarkAspectLoadingKey that = (StarlarkAspectLoadingKey) o;
- return Objects.equal(targetLabel, that.targetLabel)
+ return hashCode == that.hashCode
+ && Objects.equal(targetLabel, that.targetLabel)
&& Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
&& Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
&& Objects.equal(starlarkFileLabel, that.starlarkFileLabel)
diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractSkyKey.java b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyKey.java
index 7c7fe27..22aac53 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractSkyKey.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyKey.java
@@ -14,50 +14,40 @@
package com.google.devtools.build.skyframe;
-import com.google.common.base.Preconditions;
+import static com.google.common.base.Preconditions.checkNotNull;
/**
* For use when the {@link #argument} of the {@link SkyKey} cannot be a {@link SkyKey} itself,
* either because it is a type like List or because it is already a different {@link SkyKey}.
* Provides convenient boilerplate.
+ *
+ * <p>An argument's hash code might not be stable across JVM instances if it transitively depends on
+ * an object that uses the default identity-based {@link Object#hashCode} or {@link
+ * System#identityHashCode}. For this reason, the {@link #hashCode} field is {@code transient}.
+ * Subclasses should manage serialization (i.e. using {@code AutoCodec}) to ensure that {@link
+ * #AbstractSkyKey(Object)} is invoked on deserialization to freshly compute the hash code.
*/
public abstract class AbstractSkyKey<T> implements SkyKey {
+
// Visible for serialization.
protected final T arg;
+
/**
- * Cache the hash code for this object. It might be expensive to compute. It is transient because
- * argument's hash code might not be stable across JVM instances.
+ * Caches the hash code for this object. It might be expensive to compute.
+ *
+ * <p>The hash code is computed eagerly because it is expected that instances are interned,
+ * meaning that {@link #hashCode()} will be called immediately for every instance.
*/
- private transient int hashCode;
+ private final transient int hashCode;
protected AbstractSkyKey(T arg) {
- this.arg = Preconditions.checkNotNull(arg);
+ this.arg = checkNotNull(arg);
+ this.hashCode = 31 * functionName().hashCode() + arg.hashCode();
}
@Override
public final int hashCode() {
- // We use the hash code caching strategy employed by java.lang.String. There are three subtle
- // things going on here:
- //
- // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet.
- // Yes, this means that if the hash code is really 0 then we will "recompute" it each time. But
- // this isn't a problem in practice since a hash code of 0 should be rare.
- //
- // (2) Since we have no synchronization, multiple threads can race here thinking there are the
- // first one to compute and cache the hash code.
- //
- // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one
- // thread may not be visible by another.
- //
- // All three of these issues are benign from a correctness perspective; in the end we have no
- // overhead from synchronization, at the cost of potentially computing the hash code more than
- // once.
- int h = hashCode;
- if (h == 0) {
- h = computeHashCode();
- hashCode = h;
- }
- return h;
+ return hashCode;
}
@Override
@@ -65,10 +55,6 @@
return arg;
}
- private int computeHashCode() {
- return 31 * functionName().hashCode() + arg.hashCode();
- }
-
@Override
public final boolean equals(Object obj) {
if (this == obj) {
@@ -78,7 +64,9 @@
return false;
}
AbstractSkyKey<?> that = (AbstractSkyKey<?>) obj;
- return this.functionName().equals(that.functionName()) && this.arg.equals(that.arg);
+ return hashCode == that.hashCode
+ && functionName().equals(that.functionName())
+ && arg.equals(that.arg);
}
@Override
diff --git a/src/test/java/com/google/devtools/build/skyframe/BUILD b/src/test/java/com/google/devtools/build/skyframe/BUILD
index 1aff611..9dc2576 100644
--- a/src/test/java/com/google/devtools/build/skyframe/BUILD
+++ b/src/test/java/com/google/devtools/build/skyframe/BUILD
@@ -53,6 +53,8 @@
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
+ "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
+ "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:graph_inconsistency_java_proto",
diff --git a/src/test/java/com/google/devtools/build/skyframe/SkyKeyTest.java b/src/test/java/com/google/devtools/build/skyframe/SkyKeyTest.java
index d589a7c..2a6c089 100644
--- a/src/test/java/com/google/devtools/build/skyframe/SkyKeyTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/SkyKeyTest.java
@@ -15,9 +15,11 @@
import static com.google.common.truth.Truth.assertThat;
-import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.concurrent.BlazeInterners;
-import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.common.collect.ImmutableClassToInstanceMap;
+import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.testutils.TestUtils;
import java.io.Serializable;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -33,70 +35,64 @@
public void testHashCodeTransience() throws Exception {
// Given a freshly constructed HashCodeSpy object,
HashCodeSpy hashCodeSpy = new HashCodeSpy();
- assertThat(hashCodeSpy.getNumberOfTimesHashCodeCalled()).isEqualTo(0);
+ assertThat(hashCodeSpy.numberOfTimesHashCodeCalled).isEqualTo(0);
// When a SkyKey is constructed with that HashCodeSpy as its argument,
- SkyKey originalKey = Key.create(hashCodeSpy);
+ SkyKey originalKey = new Key(hashCodeSpy);
// Then the HashCodeSpy reports that its hashcode method was called once.
- assertThat(hashCodeSpy.getNumberOfTimesHashCodeCalled()).isEqualTo(1);
-
+ assertThat(hashCodeSpy.numberOfTimesHashCodeCalled).isEqualTo(1);
// When the SkyKey's hashCode method is called,
originalKey.hashCode();
// Then the spy's hashCode method isn't called, because the SkyKey's hashCode was cached.
- assertThat(hashCodeSpy.getNumberOfTimesHashCodeCalled()).isEqualTo(1);
-
+ assertThat(hashCodeSpy.numberOfTimesHashCodeCalled).isEqualTo(1);
// When that SkyKey is serialized and then deserialized,
- SkyKey newKey = (SkyKey) TestUtils.deserializeObject(TestUtils.serializeObject(originalKey));
+ SkyKey newKey =
+ (SkyKey)
+ TestUtils.fromBytes(
+ new DeserializationContext(ImmutableClassToInstanceMap.of()),
+ TestUtils.toBytes(
+ new SerializationContext(ImmutableClassToInstanceMap.of()), originalKey));
- // Then the new SkyKey's HashCodeSpy has not had its hashCode method called.
+ // Then the new SkyKey recomputed its hashcode on deserialization.
+ assertThat(newKey.hashCode()).isEqualTo(originalKey.hashCode());
HashCodeSpy spyInNewKey = (HashCodeSpy) newKey.argument();
- assertThat(spyInNewKey.getNumberOfTimesHashCodeCalled()).isEqualTo(0);
+ assertThat(spyInNewKey.numberOfTimesHashCodeCalled).isEqualTo(1);
-
- // When the new SkyKey's hashCode method is called once,
+ // When the new SkyKey's hashCode method is called,
newKey.hashCode();
- // Then the new SkyKey's spy's hashCode method gets called.
- assertThat(spyInNewKey.getNumberOfTimesHashCodeCalled()).isEqualTo(1);
-
-
- // When the new SkyKey's hashCode method is called a second time,
- newKey.hashCode();
-
- // Then the new SkyKey's spy's hashCOde isn't called a second time, because the SkyKey's
- // hashCode was cached.
- assertThat(spyInNewKey.getNumberOfTimesHashCodeCalled()).isEqualTo(1);
+ // Then the new SkyKey's spy's hashCode method is not called again.
+ assertThat(spyInNewKey.numberOfTimesHashCodeCalled).isEqualTo(1);
}
- private static class HashCodeSpy implements Serializable {
+ static final class HashCodeSpy implements Serializable {
private transient int numberOfTimesHashCodeCalled;
- public int getNumberOfTimesHashCodeCalled() {
- return numberOfTimesHashCodeCalled;
- }
-
@Override
public int hashCode() {
numberOfTimesHashCodeCalled++;
return 42;
}
+
+ // Implemented so that numberOfTimesHashCodeCalled is not incremented when the debugger calls
+ // toString() - the default Object#toString() calls hashCode().
+ @Override
+ public String toString() {
+ return String.format("HashCodeSpy{count=%s}", numberOfTimesHashCodeCalled);
+ }
}
- private static class Key extends AbstractSkyKey<HashCodeSpy> {
- private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
+ @AutoCodec
+ static final class Key extends AbstractSkyKey<HashCodeSpy> {
- private Key(HashCodeSpy arg) {
+ Key(HashCodeSpy arg) {
super(arg);
}
- private static Key create(HashCodeSpy arg) {
- return interner.intern(new Key(arg));
- }
-
@Override
public SkyFunctionName functionName() {
return SkyFunctionName.FOR_TESTING;
