#3876 Fix TypeFactory cache performance degradation with `constructSp…

…ecializedType` (#3875)

Improve TypeFactory cache performance with `constructSpecializedType` (...)

src/main/java/com/fasterxml/jackson/databind/JavaType.java

@@ -86,7 +86,7 @@ protected JavaType(Class<?> raw, int additionalHash,
             Object valueHandler, Object typeHandler, boolean asStatic)
     {
         _class = raw;
-        _hash = raw.getName().hashCode() + additionalHash;
+        _hash =  31 * additionalHash + raw.hashCode();
         _valueHandler = valueHandler;
         _typeHandler = typeHandler;
         _asStatic = asStatic;
@@ -645,5 +645,5 @@ public String getErasedSignature() {
     public abstract boolean equals(Object o);
 
     @Override
-    public final int hashCode() { return _hash; }
+    public int hashCode() { return _hash; }
 }

src/main/java/com/fasterxml/jackson/databind/type/IdentityEqualityType.java

@@ -0,0 +1,31 @@
+package com.fasterxml.jackson.databind.type;
+
+import com.fasterxml.jackson.databind.JavaType;
+
+/**
+ * Internal abstract type representing {@link TypeBase} implementations which use reference equality.
+ */
+abstract class IdentityEqualityType extends TypeBase {
+    protected IdentityEqualityType(
+            Class<?> raw,
+            TypeBindings bindings,
+            JavaType superClass,
+            JavaType[] superInts,
+            int hash,
+            Object valueHandler,
+            Object typeHandler,
+            boolean asStatic) {
+        super(raw, bindings, superClass, superInts, hash, valueHandler, typeHandler, asStatic);
+    }
+
+    @Override
+    public final boolean equals(Object o) {
+        return o == this;
+    }
+
+    @Override
+    public final int hashCode() {
+        // The identity hashCode must be used otherwise all instances will have colliding hashCodes.
+        return System.identityHashCode(this);
+    }
+}

src/main/java/com/fasterxml/jackson/databind/type/MapLikeType.java

@@ -36,8 +36,9 @@ protected MapLikeType(Class<?> mapType, TypeBindings bindings,
             JavaType superClass, JavaType[] superInts, JavaType keyT,
             JavaType valueT, Object valueHandler, Object typeHandler,
             boolean asStatic) {
-        super(mapType, bindings, superClass, superInts, keyT.hashCode()
-                ^ valueT.hashCode(), valueHandler, typeHandler, asStatic);
+        super(mapType, bindings, superClass, superInts,
+                31 * keyT.hashCode() + valueT.hashCode(),
+                valueHandler, typeHandler, asStatic);
         _keyType = keyT;
         _valueType = valueT;
     }

src/main/java/com/fasterxml/jackson/databind/type/PlaceholderForType.java

@@ -8,7 +8,7 @@
  *
  * @since 2.8.11
  */
-public class PlaceholderForType extends TypeBase
+public class PlaceholderForType extends IdentityEqualityType
 {
     private static final long serialVersionUID = 1L;
 
@@ -99,11 +99,6 @@ public String toString() {
         return getErasedSignature(new StringBuilder()).toString();
     }
 
-    @Override
-    public boolean equals(Object o) {
-        return (o == this);
-    }
-
     private <T> T _unsupported() {
         throw new UnsupportedOperationException("Operation should not be attempted on "+getClass().getName());
     }

src/main/java/com/fasterxml/jackson/databind/type/ResolvedRecursiveType.java

@@ -7,7 +7,7 @@
  *
  * @since 2.7
  */
-public class ResolvedRecursiveType extends TypeBase
+public class ResolvedRecursiveType extends IdentityEqualityType
 {
     private static final long serialVersionUID = 1L;
 
@@ -126,16 +126,16 @@ public String toString() {
         return sb.toString();
     }
 
-    @Override
-    public boolean equals(Object o) {
-        if (o == this) return true;
-        if (o == null) return false;
-        if (o.getClass() == getClass()) {
+    //@Override
+    //public boolean equals(Object o) {
+        //if (o == this) return true;
+        //if (o == null) return false;
+        //if (o.getClass() == getClass()) {
             // 16-Jun-2017, tatu: as per [databind#1658], cannot do recursive call since
             //    there is likely to be a cycle...
 
             // but... true or false?
-            return false;
+            //return false;
 
             /*
             // Do NOT ever match unresolved references
@@ -145,7 +145,7 @@ public boolean equals(Object o) {
             return (o.getClass() == getClass()
                     && _referencedType.equals(((ResolvedRecursiveType) o).getSelfReferencedType()));
                     */
-        }
-        return false;
-    }
+        //}
+        //return false;
+    //}
 }

src/main/java/com/fasterxml/jackson/databind/type/SimpleType.java

@@ -52,8 +52,17 @@ protected SimpleType(Class<?> cls, TypeBindings bindings,
             JavaType superClass, JavaType[] superInts,
             Object valueHandler, Object typeHandler, boolean asStatic)
     {
-        super(cls, bindings, superClass, superInts,
-                0, valueHandler, typeHandler, asStatic);
+        super(
+                cls,
+                bindings,
+                superClass,
+                superInts,
+                // TypeBase normalizes null bindings to the singleton returned by TypeBindings.emptyBindings()
+                // so we must compute the same hashCode in both cases.
+                (bindings == null ? TypeBindings.emptyBindings() : bindings).hashCode(),
+                valueHandler,
+                typeHandler,
+                asStatic);
     }
 
     /**

src/main/java/com/fasterxml/jackson/databind/type/TypeBindings.java

@@ -56,12 +56,9 @@ private TypeBindings(String[] names, JavaType[] types, String[] uvars)
         if (_names.length != _types.length) {
             throw new IllegalArgumentException("Mismatching names ("+_names.length+"), types ("+_types.length+")");
         }
-        int h = 1;
-        for (int i = 0, len = _types.length; i < len; ++i) {
-            h += _types[i].hashCode();
-        }
         _unboundVariables = uvars;
-        _hashCode = h;
+        // hashCode and equality are based solely on _types.
+        _hashCode = Arrays.hashCode(_types);
     }
 
     public static TypeBindings emptyBindings() {
@@ -253,6 +250,24 @@ public JavaType findBoundType(String name)
         return null;
     }
 
+    /**
+     * Returns true if a shallow search of the type bindings includes a placeholder
+     * type which uses reference equality, thus cannot produce cache hits. This
+     * is an optimization to avoid churning memory in the cache unnecessarily.
+     * Note that it is still possible for nested type information to contain such
+     * placeholder types (see NestedTypes1604Test for an example) so it's vital
+     * that they produce a distribution of hashCode values, even if they may push
+     * reusable data out of the cache.
+     */
+    private boolean invalidCacheKey() {
+        for (JavaType type : _types) {
+            if (type instanceof IdentityEqualityType) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     public boolean isEmpty() {
         return (_types.length == 0);
     }
@@ -309,11 +324,17 @@ public boolean hasUnbound(String name) {
      * Factory method that will create an object that can be used as a key for
      * caching purposes by {@link TypeFactory}
      *
+     * @return An object which can be used as a key in TypeFactory, or {@code null} if no key can be created.
+     *
      * @since 2.8
      */
     public Object asKey(Class<?> rawBase) {
         // safe to pass _types array without copy since it is not exposed via
         // any access, nor modified by this class
+        if (invalidCacheKey()) {
+            // If placeholders are present, no key may be returned because the key is unhelpful without context.
+            return null;
+        }
         return new AsKey(rawBase, _types, _hashCode);
     }
 
@@ -351,17 +372,8 @@ public Object asKey(Class<?> rawBase) {
             return false;
         }
         TypeBindings other = (TypeBindings) o;
-        int len = _types.length;
-        if (len != other.size()) {
-            return false;
-        }
-        JavaType[] otherTypes = other._types;
-        for (int i = 0; i < len; ++i) {
-            if (!otherTypes[i].equals(_types[i])) {
-                return false;
-            }
-        }
-        return true;
+        // hashCode and equality are based solely on _types.
+        return _hashCode == other._hashCode && Arrays.equals(_types, other._types);
     }
 
     /*
@@ -450,7 +462,7 @@ final static class AsKey {
         public AsKey(Class<?> raw, JavaType[] params, int hash) {
             _raw = raw ;
             _params = params;
-            _hash = hash;
+            _hash = 31 * raw.hashCode() + hash;
         }
 
         @Override

src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java

@@ -1465,7 +1465,7 @@ protected JavaType _fromClass(ClassStack context, Class<?> rawType, TypeBindings
         } else {
             key = bindings.asKey(rawType);
         }
-        result = _typeCache.get(key); // ok, cache object is synced
+        result = key == null ? null : _typeCache.get(key); // ok, cache object is synced
         if (result != null) {
             return result;
         }
@@ -1529,7 +1529,7 @@ else if (superClass != null) {
         context.resolveSelfReferences(result);
         // 16-Jul-2016, tatu: [databind#1302] is solved different way, but ideally we shouldn't
         //     cache anything with partially resolved `ResolvedRecursiveType`... so maybe improve
-        if (!result.hasHandlers()) {
+        if (key != null && !result.hasHandlers()) {
             _typeCache.putIfAbsent(key, result); // cache object syncs
         }
         return result;

src/test/java/com/fasterxml/jackson/databind/type/TestTypeBindings.java

@@ -5,6 +5,8 @@
 import com.fasterxml.jackson.databind.BaseMapTest;
 import com.fasterxml.jackson.databind.JavaType;
 
+import static org.junit.Assert.assertNotEquals;
+
 /**
  * Simple tests to verify for generic type binding functionality
  * implemented by {@link TypeBindings} class.
@@ -87,4 +89,40 @@ public void testInvalidBindings()
             verifyException(e, "class expects 2");
         }
     }
+
+    // for [databind#3876]
+    public void testEqualityAndHashCode()
+    {
+        JavaType stringType = DEFAULT_TF.constructType(String.class);
+        JavaType integerType = DEFAULT_TF.constructType(Integer.class);
+        TypeBindings listStringBindings = TypeBindings.create(List.class, stringType);
+        TypeBindings listStringBindingsWithUnbound = listStringBindings.withUnboundVariable("X");
+        TypeBindings iterableStringBindings = TypeBindings.create(Iterable.class, stringType);
+        TypeBindings mapStringInt = TypeBindings.create(Map.class, stringType, integerType);
+        TypeBindings mapIntString = TypeBindings.create(Map.class, integerType, stringType);
+        // Ensure that type variable names used by List and Iterable do not change in future java versions
+        assertEquals("E", listStringBindings.getBoundName(0));
+        assertEquals("T", iterableStringBindings.getBoundName(0));
+        // These TypeBindings bind the same types in the same order
+        assertEquals(listStringBindings, iterableStringBindings);
+        assertEquals(listStringBindings.hashCode(), iterableStringBindings.hashCode());
+        // Type bindings which differ by an unbound variable still evaluate to equal
+        assertEquals(listStringBindingsWithUnbound, listStringBindings);
+        assertEquals(listStringBindingsWithUnbound.hashCode(), listStringBindings.hashCode());
+        // However type bindings for the same types in different order must differ:
+        assertNotEquals(mapStringInt, mapIntString);
+        assertNotEquals(mapStringInt.hashCode(), mapIntString.hashCode());
+
+        Object iterableStringBaseList = iterableStringBindings.asKey(List.class);
+        Object listStringBaseList = listStringBindings.asKey(List.class);
+        Object listStringBindingsWithUnboundBaseList = listStringBindingsWithUnbound.asKey(List.class);
+        Object mapStringIntBaseMap = mapStringInt.asKey(Map.class);
+        Object mapIntStringBaseMap = mapIntString.asKey(Map.class);
+        assertEquals(iterableStringBaseList, listStringBaseList);
+        assertEquals(iterableStringBaseList.hashCode(), listStringBaseList.hashCode());
+        assertEquals(listStringBindingsWithUnboundBaseList, listStringBaseList);
+        assertEquals(listStringBindingsWithUnboundBaseList.hashCode(), listStringBaseList.hashCode());
+        assertNotEquals(mapStringIntBaseMap, mapIntStringBaseMap);
+        assertNotEquals(mapStringIntBaseMap.hashCode(), mapIntStringBaseMap.hashCode());
+    }
 }

src/test/java/com/fasterxml/jackson/databind/type/TestTypeFactory.java

@@ -7,6 +7,8 @@
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.*;
 
+import static org.junit.Assert.assertNotEquals;
+
 /**
  * Simple tests to verify that the {@link TypeFactory} constructs
  * type information as expected.
@@ -355,6 +357,17 @@ public void testCollectionTypesRefined()
         assertEquals(AbstractList.class, superType.getRawClass());
     }
 
+    // for [databind#3876]
+    @SuppressWarnings("rawtypes")
+    public void testCollectionsHashCode()
+    {
+        TypeFactory tf = newTypeFactory();
+        JavaType listOfCollection = tf.constructType(new TypeReference<List<Collection>>() { });
+        JavaType collectionOfList = tf.constructType(new TypeReference<Collection<List>>() { });
+        assertNotEquals(listOfCollection, collectionOfList);
+        assertNotEquals(listOfCollection.hashCode(), collectionOfList.hashCode());
+    }
+
     /*
     /**********************************************************
     /* Unit tests: map type parameter resolution
@@ -397,6 +410,24 @@ public void testMaps()
         assertEquals(tf.constructType(Boolean.class), mapType.getContentType());
     }
 
+    // for [databind#3876]
+    public void testMapsHashCode()
+    {
+        TypeFactory tf = newTypeFactory();
+        JavaType mapStringInt = tf.constructType(new TypeReference<Map<String,Integer>>() {});
+        JavaType mapIntString = tf.constructType(new TypeReference<Map<Integer,String>>() {});
+        assertNotEquals(mapStringInt, mapIntString);
+        assertNotEquals(
+                "hashCode should depend on parameter order",
+                mapStringInt.hashCode(),
+                mapIntString.hashCode());
+
+        JavaType mapStringString = tf.constructType(new TypeReference<Map<String,String>>() {});
+        JavaType mapIntInt = tf.constructType(new TypeReference<Map<Integer,Integer>>() {});
+        assertNotEquals(mapStringString, mapIntInt);
+        assertNotEquals(mapStringString.hashCode(), mapIntInt.hashCode());
+    }
+
     // since 2.7
     public void testMapTypesRefined()
     {
@@ -671,4 +702,20 @@ public void testParameterizedClassType() {
         assertEquals(1, t.containedTypeCount());
         assertEquals(CharSequence.class, t.containedType(0).getRawClass());
     }
+
+    // for [databind#3876]
+    public void testParameterizedSimpleType() {
+        TypeFactory tf = TypeFactory.defaultInstance();
+
+        JavaType charSequenceClass = tf.constructType(new TypeReference<Class<? extends CharSequence>>() { });
+        JavaType numberClass = tf.constructType(new TypeReference<Class<? extends Number>>() { });
+
+        assertEquals(SimpleType.class, charSequenceClass.getClass());
+        assertEquals(SimpleType.class, numberClass.getClass());
+
+        assertNotEquals(charSequenceClass, numberClass);
+        assertNotEquals(
+                "hash values should be distributed",
+                charSequenceClass.hashCode(), numberClass.hashCode());
+    }
 }