Use static creation method for SkyKey. This allows interning SkyKeys as they are created, as opposed to when they are requested from the ParallelEvaluator. That delay can lead to large memory spikes and churn.

--
MOS_MIGRATED_REVID=116224565

4 files changed, 13 insertions, 39 deletions

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 3d5ede4b11..c0544dfbf5 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
@@ -22,7 +22,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 = new SkyKey(FUNCTION_NAME, "ERROR_TRANSIENCE");
+  public static final SkyKey KEY = SkyKey.create(FUNCTION_NAME, "ERROR_TRANSIENCE");
   public static final ErrorTransienceValue INSTANCE = new ErrorTransienceValue();
 
   private ErrorTransienceValue() {}
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index 1899bcc490..25c3215c5c 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -13,8 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
-import static com.google.devtools.build.skyframe.SkyKeyInterner.SKY_KEY_INTERNER;
-
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Predicate;
 import com.google.common.base.Supplier;
@@ -414,12 +412,6 @@ public final class ParallelEvaluator implements Evaluator {
     protected ImmutableMap<SkyKey, ValueOrUntypedException> getValueOrUntypedExceptions(
         Set<SkyKey> depKeys) {
       checkActive();
-      Set<SkyKey> keys = Sets.newLinkedHashSetWithExpectedSize(depKeys.size());
-      for (SkyKey depKey : depKeys) {
-        // Canonicalize SkyKeys to save memory.
-        keys.add(SKY_KEY_INTERNER.intern(depKey));
-      }
-      depKeys = keys;
       Map<SkyKey, ValueWithMetadata> values = getValuesMaybeFromError(depKeys, bubbleErrorInfo);
       ImmutableMap.Builder<SkyKey, ValueOrUntypedException> builder = ImmutableMap.builder();
       for (SkyKey depKey : depKeys) {
@@ -1637,7 +1629,7 @@ public final class ParallelEvaluator implements Evaluator {
    * ArrayDeque does not permit null elements.
    */
   private static final SkyKey CHILDREN_FINISHED =
-      new SkyKey(SkyFunctionName.create("MARKER"), "MARKER");
+      SkyKey.create(SkyFunctionName.create("MARKER"), "MARKER");
 
   /** 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;
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyKey.java b/src/main/java/com/google/devtools/build/skyframe/SkyKey.java
index 482940d785..bc15bf8b3c 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyKey.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyKey.java
@@ -14,6 +14,8 @@
 package com.google.devtools.build.skyframe;
 
 import com.google.common.base.Function;
+import com.google.common.collect.Interner;
+import com.google.common.collect.Interners;
 import com.google.devtools.build.lib.util.Preconditions;
 
 import java.io.Serializable;
@@ -22,6 +24,8 @@ import java.io.Serializable;
  * A {@link SkyKey} is effectively a pair (type, name) that identifies a Skyframe value.
  */
 public final class SkyKey implements Serializable {
+  private static final Interner<SkyKey> SKY_KEY_INTERNER = Interners.newWeakInterner();
+
   private final SkyFunctionName functionName;
 
   /**
@@ -39,15 +43,20 @@ public final class SkyKey implements Serializable {
    */
   private transient int hashCode;
 
-  public SkyKey(SkyFunctionName functionName, Object valueName) {
+  private SkyKey(SkyFunctionName functionName, Object argument) {
     this.functionName = Preconditions.checkNotNull(functionName);
-    this.argument = Preconditions.checkNotNull(valueName);
+    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();
   }
 
+  public static SkyKey create(SkyFunctionName functionName, Object argument) {
+    // Intern to save memory.
+    return SKY_KEY_INTERNER.intern(new SkyKey(functionName, argument));
+  }
+
   public SkyFunctionName functionName() {
     return functionName;
   }
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyKeyInterner.java b/src/main/java/com/google/devtools/build/skyframe/SkyKeyInterner.java
deleted file mode 100644
index bc8a950899..0000000000
--- a/src/main/java/com/google/devtools/build/skyframe/SkyKeyInterner.java
+++ /dev/null
@@ -1,27 +0,0 @@
-// Copyright 2014 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.collect.Interner;
-import com.google.common.collect.Interners;
-
-/**
- * A holder class for SkyKey interning.
- */
-public class SkyKeyInterner {
-  public static final Interner<SkyKey> SKY_KEY_INTERNER =  Interners.newWeakInterner();
-
-  private SkyKeyInterner() {
-  }
-}