Changes the collection used to track subtypes in FunctionType from …

…a list to a set to reduce the cost of checking `contains()`.

The original list-based implementation was making a large number of relatively expensive `contatins()` calls in order to prevent duplication in the list. Under some assumptions about the stability of `hashCode`, a set based implementation is much faster.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=218728724

src/com/google/javascript/rhino/CyclicSerializableLinkedHashSet.java

@@ -0,0 +1,144 @@
+/*
+ *
+ * ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (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.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Rhino code, released
+ * May 6, 1999.
+ *
+ * The Initial Developer of the Original Code is
+ * Netscape Communications Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 1997-1999
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Google Inc.
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * the GNU General Public License Version 2 or later (the "GPL"), in which
+ * case the provisions of the GPL are applicable instead of those above. If
+ * you wish to allow use of your version of this file only under the terms of
+ * the GPL and not to allow others to use your version of this file under the
+ * MPL, indicate your decision by deleting the provisions above and replacing
+ * them with the notice and other provisions required by the GPL. If you do
+ * not delete the provisions above, a recipient may use your version of this
+ * file under either the MPL or the GPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+package com.google.javascript.rhino;
+
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.common.annotations.GwtIncompatible;
+import com.google.common.collect.ForwardingSet;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+import javax.annotation.Nullable;
+
+/**
+ * A {@link LinkedHashSet} that can be safely (de)serialized when involved in a reference cycle.
+ *
+ * <p>The motivating usecase of this class is the afformentioned cycles. Consider the following
+ * example:
+ *
+ * <pre>{@code
+ * class Foo implements Serializable {
+ *   private LinkedHashSet<Foo> children = new LinkedHashSet<>();
+ *
+ *   private Object hashSource = new Object();
+ *
+ *   public void addChild(Foo child) {
+ *     children.addChild(child);
+ *   }
+ *
+ *   {@literal @}Override
+ *   public int hashCode() {
+ *     return hashSouce.hashCode();
+ *   }
+ * }
+ *
+ * ****
+ *
+ *   Foo f = new Foo();
+ *   f.addChild(f);
+ *
+ * ****
+ * }</pre>
+ *
+ * If {@code f} were serialized and reserialized, there is a possiblity of exceptions via the
+ * following steps. This class is designed to eliminate such issue.
+ *
+ * <ol>
+ *   <li>{@code f} begins deserialization
+ *   <li>{@code f.children} begins deserialization
+ *   <li>{@code f.children} attempts to add {@code f} to itself
+ *   <li>{@code f.hashCode()} is called
+ *   <li>{@code f.hashSource} is {@code null}, throwing a {@code NullPointerException}
+ * </ol>
+ */
+public final class CyclicSerializableLinkedHashSet<T> extends ForwardingSet<T>
+    implements Serializable {
+
+  // This field will be non-null between `readObject` and `lazyCompleteDeserialization`.
+  @Nullable private transient List<T> deserializedElementList;
+
+  // This field will be null between `readObject` and `lazyCompleteDeserialization`.
+  @Nullable private transient LinkedHashSet<T> backingSet = new LinkedHashSet<>();
+
+  @Override
+  protected Set<T> delegate() {
+    lazyCompleteDeserialization();
+    return backingSet;
+  }
+
+  @GwtIncompatible
+  private void writeObject(ObjectOutputStream out) throws IOException {
+    // Serialize the contents of this set as a list so that they can be deserialized without
+    // immediately reconstructing the set. We want to prevent `hashCode` from being called on the
+    // elements until deserialization is totally done.
+    out.writeObject(new ArrayList<>(this));
+  }
+
+  @GwtIncompatible
+  @SuppressWarnings("unchecked")
+  private void readObject(ObjectInputStream in) throws ClassNotFoundException, IOException {
+    // Deserialize the contents of this set from a list so that we don't immediately reconstruct the
+    // set. We want to prevent `hashCode` from being called on the elements until deserialization is
+    // totally done.
+    deserializedElementList = (ArrayList<T>) in.readObject();
+  }
+
+  private void lazyCompleteDeserialization() {
+    if (!deserializationAwaitingCompletion()) {
+      checkState(
+          backingSet != null,
+          "Deserialization is not awaiting completion, but the backing set is not initialized. "
+              + "Is another class calling methods on this object during deserialization?");
+      return;
+    }
+
+    backingSet = new LinkedHashSet<>();
+    backingSet.addAll(deserializedElementList);
+    deserializedElementList = null;
+  }
+
+  private boolean deserializationAwaitingCompletion() {
+    return deserializedElementList != null;
+  }
+}

src/com/google/javascript/rhino/jstype/FunctionType.java

@@ -42,16 +42,20 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.javascript.rhino.jstype.JSTypeNative.OBJECT_TYPE;
 import static com.google.javascript.rhino.jstype.JSTypeNative.U2U_CONSTRUCTOR_TYPE;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.javascript.rhino.CyclicSerializableLinkedHashSet;
 import com.google.javascript.rhino.ErrorReporter;
 import com.google.javascript.rhino.Node;
 import com.google.javascript.rhino.Token;
+import com.google.javascript.rhino.jstype.JSType.EqCache;
+import com.google.javascript.rhino.jstype.JSType.SubtypingMode;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -63,6 +67,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import javax.annotation.Nullable;
 
 /**
  * This derived type provides extended information about a function, including its return type and
@@ -144,10 +149,13 @@ enum ConstructorAmbiguity {
   private ImmutableList<ObjectType> extendedInterfaces = ImmutableList.of();
 
   /**
-   * The types which are subtypes of this function. It is only relevant for constructors and may be
-   * {@code null}.
+   * For the instance type of this ctor, the ctor types of all known subtypes of that instance type.
+   *
+   * <p>This field is only applicable to ctor functions.
    */
-  private List<FunctionType> subTypes;
+  @Nullable
+  // @MonotonicNonNull
+  private CyclicSerializableLinkedHashSet<FunctionType> knownSubtypeCtors;
 
   /** Creates an instance for a function that might be a constructor. */
   FunctionType(
@@ -499,13 +507,13 @@ private boolean setPrototypeNoCheck(ObjectType prototype, Node propertyNode) {
     if (isConstructor() || isInterface()) {
       FunctionType superClass = getSuperClassConstructor();
       if (superClass != null) {
-        superClass.addSubType(this);
+        superClass.addSubTypeIfNotPresent(this);
       }
 
       if (isInterface()) {
         for (ObjectType interfaceType : getExtendedInterfaces()) {
           if (interfaceType.getConstructor() != null) {
-            interfaceType.getConstructor().addSubType(this);
+            interfaceType.getConstructor().addSubTypeIfNotPresent(this);
           }
         }
       }
@@ -1124,18 +1132,15 @@ public final void setSource(Node source) {
     this.source = source;
   }
 
-  final void addSubTypeIfNotPresent(FunctionType subType) {
-    if (subTypes == null || !subTypes.contains(subType)) {
-      addSubType(subType);
-    }
-  }
+  /** Adds a type to the set of known subtype ctors for this type. */
+  final void addSubTypeIfNotPresent(FunctionType subtype) {
+    checkState(isConstructor() || isInterface());
 
-  /** Adds a type to the list of subtypes for this type. */
-  private void addSubType(FunctionType subType) {
-    if (subTypes == null) {
-      subTypes = new ArrayList<>();
+    if (knownSubtypeCtors == null) {
+      knownSubtypeCtors = new CyclicSerializableLinkedHashSet<>();
     }
-    subTypes.add(subType);
+
+    knownSubtypeCtors.add(subtype);
   }
 
   // NOTE(sdh): One might assume that immediately after calling this, hasCachedValues() should
@@ -1145,8 +1150,8 @@ private void addSubType(FunctionType subType) {
   public final void clearCachedValues() {
     super.clearCachedValues();
 
-    if (subTypes != null) {
-      for (FunctionType subType : subTypes) {
+    if (knownSubtypeCtors != null) {
+      for (FunctionType subType : knownSubtypeCtors) {
         subType.clearCachedValues();
       }
     }
@@ -1164,7 +1169,7 @@ public final void clearCachedValues() {
 
   public final Iterable<FunctionType> getDirectSubTypes() {
     return Iterables.concat(
-        subTypes != null ? subTypes : ImmutableList.<FunctionType>of(),
+        knownSubtypeCtors != null ? knownSubtypeCtors : ImmutableList.of(),
         this.registry.getDirectImplementors(this));
   }
 
@@ -1210,11 +1215,8 @@ JSType resolveInternal(ErrorReporter reporter) {
       extendedInterfaces = resolvedExtended;
     }
 
-    if (subTypes != null) {
-      for (int i = 0; i < subTypes.size(); i++) {
-        FunctionType subType = subTypes.get(i);
-        subTypes.set(i, JSType.toMaybeFunctionType(subType.resolve(reporter)));
-      }
+    if (knownSubtypeCtors != null) {
+      resolveKnownSubtypeCtors(reporter);
     }
 
     return super.resolveInternal(reporter);
@@ -1245,6 +1247,30 @@ private ImmutableList<ObjectType> resolveTypeListHelper(
     return changed ? resolvedList.build() : null;
   }
 
+  private void resolveKnownSubtypeCtors(ErrorReporter reporter) {
+    // We want to resolve all of the known subtypes-ctors so we can store those resolved types
+    // instead.
+    ImmutableList<FunctionType> setCopy;
+    do {
+      setCopy =
+          // However, resolving a type may cause more subtype-ctors to be registered. To avoid a
+          // `ConcurrentModificationException` we operate on a copy of the original set. We leave
+          // the original set in place in case resolution would re-add a previously known type.
+          ImmutableList.copyOf(knownSubtypeCtors).stream()
+              .map((t) -> JSType.toMaybeFunctionType(t.resolve(reporter)))
+              .collect(toImmutableList());
+
+      // We do this iteratively until resolving adds no more subtypes.
+    } while (setCopy.size() != knownSubtypeCtors.size());
+
+    // Additionally, resolving a type may change its `hashCode`, so we have to rebuild the set of
+    // subtype-ctors.
+    knownSubtypeCtors = null; // Reset
+    for (FunctionType subtypeCtor : setCopy) {
+      addSubTypeIfNotPresent(subtypeCtor);
+    }
+  }
+
   @Override
   public final String toDebugHashCodeString() {
     if (this == registry.getNativeType(JSTypeNative.FUNCTION_INSTANCE_TYPE)) {