Replace LegacySkyKey by AbstractSkyKey or custom SkyKeys. AbstractSkyKey doesn't save memory in the 32-bit case, but makes it easier for people to see how many SkyKeys we have.

There's some unnecessary interning in tests, but it was easier to copypasta and doesn't harm anything, I think.

PiperOrigin-RevId: 187694309

5 files changed, 91 insertions, 132 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractSkyKey.java b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyKey.java
new file mode 100644
index 0000000000..7c7fe27029
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyKey.java
@@ -0,0 +1,88 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.skyframe;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * 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.
+ */
+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.
+   */
+  private transient int hashCode;
+
+  protected AbstractSkyKey(T arg) {
+    this.arg = Preconditions.checkNotNull(arg);
+  }
+
+  @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;
+  }
+
+  @Override
+  public final T argument() {
+    return arg;
+  }
+
+  private int computeHashCode() {
+    return 31 * functionName().hashCode() + arg.hashCode();
+  }
+
+  @Override
+  public final boolean equals(Object obj) {
+    if (this == obj) {
+      return true;
+    }
+    if (obj == null || getClass() != obj.getClass()) {
+      return false;
+    }
+    AbstractSkyKey<?> that = (AbstractSkyKey<?>) obj;
+    return this.functionName().equals(that.functionName()) && this.arg.equals(that.arg);
+  }
+
+  @Override
+  public String toString() {
+    return functionName() + ":" + arg;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/skyframe/BUILD b/src/main/java/com/google/devtools/build/skyframe/BUILD
index c33e8ae265..bb24dde0c1 100644
--- a/src/main/java/com/google/devtools/build/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/skyframe/BUILD
@@ -7,7 +7,7 @@ package(
 SKYFRAME_OBJECT_SRCS = [
     "SkyValue.java",
     "SkyKey.java",
-    "LegacySkyKey.java",
+    "AbstractSkyKey.java",
     "SkyFunctionName.java",
 ]
 
@@ -17,7 +17,6 @@ java_library(
     visibility = ["//visibility:public"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/collect",
-        "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//third_party:guava",
     ],
 )
diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
index 6d44fbe47c..babf90208c 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
@@ -23,7 +23,7 @@ import java.io.ObjectOutputStream;
  */
 public final class ErrorTransienceValue implements SkyValue {
   public static final SkyFunctionName FUNCTION_NAME = SkyFunctionName.create("ERROR_TRANSIENCE");
-  public static final SkyKey KEY = LegacySkyKey.create(FUNCTION_NAME, "ERROR_TRANSIENCE");
+  @AutoCodec public static final SkyKey KEY = () -> FUNCTION_NAME;
   @AutoCodec public static final ErrorTransienceValue INSTANCE = new ErrorTransienceValue();
 
   private ErrorTransienceValue() {}
diff --git a/src/main/java/com/google/devtools/build/skyframe/LegacySkyKey.java b/src/main/java/com/google/devtools/build/skyframe/LegacySkyKey.java
deleted file mode 100644
index 311c892f20..0000000000
--- a/src/main/java/com/google/devtools/build/skyframe/LegacySkyKey.java
+++ /dev/null
@@ -1,127 +0,0 @@
-// Copyright 2017 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.skyframe;
-
-import com.google.common.base.Preconditions;
-import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.concurrent.BlazeInterners;
-
-/**
- * Basic implementation of {@link SkyKey}. Potentially non-optimal from a memory perspective, since
- * it uses fields for hash code and {@link #functionName}. The latter should be implemented instead
- * using polymorphism. See {@code ArtifactSkyKey} for an example.
- */
-public class LegacySkyKey implements SkyKey {
-  private static final Interner<SkyKey> SKY_KEY_INTERNER = BlazeInterners.newWeakInterner();
-
-  /**
-   * Creates a {@link SkyKey}. Prefer instead creating custom SkyKeys that are their own arguments,
-   * saving the object wrapper. See {@code ArtifactSkyKey} for an example.
-   */
-  // TODO(janakr): migrate users of this to use custom SkyKey subclasses and delete this.
-  @Deprecated
-  public static SkyKey create(SkyFunctionName functionName, Object argument) {
-    // Intern to save memory.
-    return SKY_KEY_INTERNER.intern(new LegacySkyKey(functionName, argument));
-  }
-
-  private final SkyFunctionName functionName;
-
-  /**
-   * The name of the value.
-   *
-   * <p>This is deliberately an untyped Object so that we can use arbitrary value types (e.g.,
-   * Labels, PathFragments, BuildConfigurations, etc.) as value names without incurring
-   * serialization costs in the in-memory implementation of the graph.
-   */
-  private final Object argument;
-
-  /**
-   * 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.
-   */
-  private transient int hashCode;
-
-  private LegacySkyKey(SkyFunctionName functionName, Object argument) {
-    this.functionName = Preconditions.checkNotNull(functionName);
-    this.argument = Preconditions.checkNotNull(argument);
-    // 'hashCode' is non-volatile and non-final, so this write may in fact *not* be visible to other
-    // threads. But this isn't a concern from a correctness perspective. See the comments in
-    // #hashCode for more details.
-    this.hashCode = computeHashCode();
-  }
-
-  @Override
-  public SkyFunctionName functionName() {
-    return functionName;
-  }
-
-  @Override
-  public Object argument() {
-    return argument;
-  }
-
-  @Override
-  public String toString() {
-    return functionName + ":" + argument;
-  }
-
-  @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 31 * functionName.hashCode() + argument.hashCode();
-  }
-
-  @Override
-  public boolean equals(Object obj) {
-    if (this == obj) {
-      return true;
-    }
-    if (obj == null) {
-      return false;
-    }
-    if (getClass() != obj.getClass()) {
-      return false;
-    }
-    LegacySkyKey other = (LegacySkyKey) obj;
-    if (hashCode() != other.hashCode()) {
-      return false;
-    }
-    return functionName.equals(other.functionName) && argument.equals(other.argument);
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java b/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
index ad952c2ef6..d1348daab2 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
@@ -77,8 +77,7 @@ public class SimpleCycleDetector implements CycleDetector {
    * value is popped, we know that all the children are finished. We would use null instead, but
    * ArrayDeque does not permit null elements.
    */
-  private static final SkyKey CHILDREN_FINISHED =
-      LegacySkyKey.create(SkyFunctionName.create("MARKER"), "MARKER");
+  private static final SkyKey CHILDREN_FINISHED = () -> null;
 
   /** The max number of cycles we will report to the user for a given root, to avoid OOMing. */
   private static final int MAX_CYCLES = 20;