Update the global cache from a synchronized map to a concurrent map

Author: garydgregory

src/main/java/org/apache/commons/beanutils2/MethodUtils.java

@@ -17,17 +17,15 @@
 
 package org.apache.commons.beanutils2;
 
-import java.lang.ref.Reference;
-import java.lang.ref.WeakReference;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.Map;
 import java.util.Objects;
-import java.util.WeakHashMap;
+import java.util.function.Function;
 
-import org.apache.commons.lang3.ClassUtils;
+import org.apache.commons.collections4.map.ConcurrentReferenceHashMap;
+import org.apache.commons.collections4.map.ConcurrentReferenceHashMap.ReferenceType;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
@@ -57,6 +55,7 @@ public final class MethodUtils {
      * Represents the key to looking up a Method by reflection.
      */
     private static final class MethodKey {
+
         private final Class<?> cls;
         private final String methodName;
         private final Class<?>[] paramTypes;
@@ -104,6 +103,11 @@ public boolean equals(final Object obj) {
         public int hashCode() {
             return hashCode;
         }
+
+        @Override
+        public String toString() {
+            return "MethodKey [cls=" + cls + ", methodName=" + methodName + ", paramTypes=" + Arrays.toString(paramTypes) + ", exact=" + exact + "]";
+        }
     }
 
     private static final Log LOG = LogFactory.getLog(MethodUtils.class);
@@ -115,35 +119,28 @@ public int hashCode() {
      * would mean having a map keyed by context classloader which may introduce memory-leak problems.
      * </p>
      */
-    private static boolean CACHE_METHODS = true;
+    private static boolean CACHE_ENABLED = true;
 
     /**
-     * Stores a cache of MethodDescriptor -> Method in a WeakHashMap.
+     * Stores a cache of MethodKey -> Method.
      * <p>
      * The keys into this map only ever exist as temporary variables within methods of this class, and are never exposed to users of this class. This means that
-     * the WeakHashMap is used only as a mechanism for limiting the size of the cache, that is, a way to tell the garbage collector that the contents of the
-     * cache can be completely garbage-collected whenever it needs the memory. Whether this is a good approach to this problem is doubtful; something like the
-     * commons-collections LRUMap may be more appropriate (though of course selecting an appropriate size is an issue).
+     * this map is used only as a mechanism for limiting the size of the cache, that is, a way to tell the garbage collector that the contents of the cache can
+     * be completely garbage-collected whenever it needs the memory. Whether this is a good approach to this problem is doubtful; something like the Commons
+     * Collections LRUMap may be more appropriate (though of course selecting an appropriate size is an issue).
      * </p>
      * <p>
-     * This static variable is safe even when this code is deployed via a shared classloader because it is keyed via a MethodDescriptor object which has a Class
-     * as one of its members and that member is used in the MethodDescriptor.equals method. So two components that load the same class via different class
-     * loaders will generate non-equal MethodDescriptor objects and hence end up with different entries in the map.
+     * This static variable is safe even when this code is deployed via a shared class loader because it is keyed via a MethodKey object which has a Class as
+     * one of its members and that member is used in the MethodKey.equals method. So two components that load the same class via different class loaders will
+     * generate non-equal MethodKey objects and hence end up with different entries in the map.
      * </p>
      */
-    private static final Map<MethodKey, Reference<Method>> CACHE = Collections.synchronizedMap(new WeakHashMap<>());
-
-    /**
-     * Add a method to the cache.
-     *
-     * @param key     The method descriptor.
-     * @param method The method to cache.
-     */
-    private static void cacheMethod(final MethodKey key, final Method method) {
-        if (CACHE_METHODS && method != null) {
-            CACHE.put(key, new WeakReference<>(method));
-        }
-    }
+    // @formatter:off
+    private static final Map<MethodKey, Method> CACHE = new ConcurrentReferenceHashMap.Builder<MethodKey, Method>()
+            .setKeyReferenceType(ReferenceType.WEAK)
+            .setValueReferenceType(ReferenceType.WEAK)
+            .get();
+    // @formatter:on
 
     /**
      * Clear the method cache.
@@ -157,9 +154,17 @@ public static synchronized int clearCache() {
         return size;
     }
 
+    private static Method computeIfAbsent(final MethodKey key, final Function<MethodKey, ? extends Method> mappingFunction) {
+        final Method method = CACHE_ENABLED ? CACHE.computeIfAbsent(key, k -> mappingFunction.apply(k)) : mappingFunction.apply(key);
+        if (LOG.isTraceEnabled()) {
+            LOG.trace("Matched " + key + " with method: " + method + ", CACHE_ENABLED: " + CACHE_ENABLED);
+        }
+        return method;
+    }
+
     /**
-     * Gets an accessible method (that is, one that can be invoked via reflection) that implements the specified Method. If no such method can be found,
-     * return {@code null}.
+     * Gets an accessible method (that is, one that can be invoked via reflection) that implements the specified Method. If no such method can be found, return
+     * {@code null}.
      *
      * @param clazz  The class of the object.
      * @param method The method that we wish to call.
@@ -171,12 +176,10 @@ public static Method getAccessibleMethod(Class<?> clazz, Method method) {
         if (method == null) {
             return null;
         }
-
         // If the requested method is not public we cannot call it
         if (!Modifier.isPublic(method.getModifiers())) {
             return null;
         }
-
         boolean sameClass = true;
         if (clazz == null) {
             clazz = method.getDeclaringClass();
@@ -186,26 +189,21 @@ public static Method getAccessibleMethod(Class<?> clazz, Method method) {
             }
             sameClass = clazz.equals(method.getDeclaringClass());
         }
-
         // If the class is public, we are done
         if (Modifier.isPublic(clazz.getModifiers())) {
             if (!sameClass && !Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
                 setMethodAccessible(method); // Default access superclass workaround
             }
             return method;
         }
-
         final String methodName = method.getName();
         final Class<?>[] parameterTypes = method.getParameterTypes();
-
         // Check the implemented interfaces and subinterfaces
         method = getAccessibleMethodFromInterfaceNest(clazz, methodName, parameterTypes);
-
         // Check the superclass chain
         if (method == null) {
             method = getAccessibleMethodFromSuperclass(clazz, methodName, parameterTypes);
         }
-
         return method;
     }
 
@@ -219,24 +217,18 @@ public static Method getAccessibleMethod(Class<?> clazz, Method method) {
      * @return The accessible method.
      */
     public static Method getAccessibleMethod(final Class<?> clazz, final String methodName, final Class<?>... parameterTypes) {
-        try {
-            final MethodKey key = new MethodKey(clazz, methodName, parameterTypes, true);
-            // Check the cache first
-            Method method = getCachedMethod(key);
-            if (method != null) {
-                return method;
+        return computeIfAbsent(new MethodKey(clazz, methodName, parameterTypes, true), k -> {
+            try {
+                return getAccessibleMethod(clazz, clazz.getMethod(methodName, parameterTypes));
+            } catch (final NoSuchMethodException e) {
+                return null;
             }
-            method = getAccessibleMethod(clazz, clazz.getMethod(methodName, parameterTypes));
-            cacheMethod(key, method);
-            return method;
-        } catch (final NoSuchMethodException e) {
-            return null;
-        }
+        });
     }
 
     /**
-     * Gets an accessible method (that is, one that can be invoked via reflection) that implements the specified Method. If no such method can be found,
-     * return {@code null}.
+     * Gets an accessible method (that is, one that can be invoked via reflection) that implements the specified Method. If no such method can be found, return
+     * {@code null}.
      *
      * @param method The method that we wish to call.
      * @return The accessible method.
@@ -292,8 +284,8 @@ private static Method getAccessibleMethodFromInterfaceNest(Class<?> clazz, final
     }
 
     /**
-     * Gets an accessible method (that is, one that can be invoked via reflection) by scanning through the superclasses. If no such method can be found,
-     * return {@code null}.
+     * Gets an accessible method (that is, one that can be invoked via reflection) by scanning through the superclasses. If no such method can be found, return
+     * {@code null}.
      *
      * @param clazz          Class to be checked.
      * @param methodName     Method name of the method we wish to call.
@@ -314,22 +306,6 @@ private static Method getAccessibleMethodFromSuperclass(final Class<?> clazz, fi
         return null;
     }
 
-    /**
-     * Gets the method from the cache, if present.
-     *
-     * @param key The method descriptor.
-     * @return The cached method.
-     */
-    private static Method getCachedMethod(final MethodKey key) {
-        if (CACHE_METHODS) {
-            final Reference<Method> methodRef = CACHE.get(key);
-            if (methodRef != null) {
-                return methodRef.get();
-            }
-        }
-        return null;
-    }
-
     /**
      * Gets an accessible method that matches the given name and has compatible parameters. Compatible parameters mean that every method parameter is assignable
      * from the given parameters. In other words, it finds a method with the given name that will take the parameters given.
@@ -341,152 +317,21 @@ private static Method getCachedMethod(final MethodKey key) {
      * <p>
      * This method can match primitive parameter by passing in wrapper classes. For example, a {@code Boolean</code> will match a primitive <code>boolean}
      * parameter.
+     * </p>
      *
      * @param clazz          find method in this class.
      * @param methodName     find method with this name.
      * @param parameterTypes find method with compatible parameters.
      * @return The accessible method.
      */
     public static Method getMatchingAccessibleMethod(final Class<?> clazz, final String methodName, final Class<?>[] parameterTypes) {
-        // trace logging
-        if (LOG.isTraceEnabled()) {
-            LOG.trace("Matching name=" + methodName + " on " + clazz);
-        }
-        final MethodKey key = new MethodKey(clazz, methodName, parameterTypes, false);
-        // see if we can find the method directly
-        // most of the time this works and it's much faster
-        try {
-            // Check the cache first
-            Method method = getCachedMethod(key);
+        return computeIfAbsent(new MethodKey(clazz, methodName, parameterTypes, false), k -> {
+            final Method method = org.apache.commons.lang3.reflect.MethodUtils.getMatchingAccessibleMethod(clazz, methodName, parameterTypes);
             if (method != null) {
-                return method;
-            }
-            method = clazz.getMethod(methodName, parameterTypes);
-            if (LOG.isTraceEnabled()) {
-                LOG.trace("Found straight match: " + method);
-                LOG.trace("isPublic:" + Modifier.isPublic(method.getModifiers()));
+                setMethodAccessible(method); // Default access superclass workaround
             }
-            setMethodAccessible(method); // Default access superclass workaround
-            cacheMethod(key, method);
             return method;
-        } catch (final NoSuchMethodException e) {
-            /* SWALLOW */ }
-        // search through all methods
-        final int paramSize = parameterTypes.length;
-        Method bestMatch = null;
-        final Method[] methods = clazz.getMethods();
-        float bestMatchCost = Float.MAX_VALUE;
-        float myCost = Float.MAX_VALUE;
-        for (final Method method2 : methods) {
-            if (method2.getName().equals(methodName)) {
-                // log some trace information
-                if (LOG.isTraceEnabled()) {
-                    LOG.trace("Found matching name:");
-                    LOG.trace(method2);
-                }
-                // compare parameters
-                final Class<?>[] methodsParams = method2.getParameterTypes();
-                final int methodParamSize = methodsParams.length;
-                if (methodParamSize == paramSize) {
-                    boolean match = true;
-                    for (int n = 0; n < methodParamSize; n++) {
-                        if (LOG.isTraceEnabled()) {
-                            LOG.trace("Param=" + parameterTypes[n].getName());
-                            LOG.trace("Method=" + methodsParams[n].getName());
-                        }
-                        if (!ClassUtils.isAssignable(parameterTypes[n], methodsParams[n])) {
-                            if (LOG.isTraceEnabled()) {
-                                LOG.trace(methodsParams[n] + " is not assignable from " + parameterTypes[n]);
-                            }
-                            match = false;
-                            break;
-                        }
-                    }
-                    if (match) {
-                        // get accessible version of method
-                        final Method method = getAccessibleMethod(clazz, method2);
-                        if (method != null) {
-                            if (LOG.isTraceEnabled()) {
-                                LOG.trace(method + " accessible version of " + method2);
-                            }
-                            setMethodAccessible(method); // Default access superclass workaround
-                            myCost = getTotalTransformationCost(parameterTypes, method.getParameterTypes());
-                            if (myCost < bestMatchCost) {
-                                bestMatch = method;
-                                bestMatchCost = myCost;
-                            }
-                        }
-                        LOG.trace("Couldn't find accessible method.");
-                    }
-                }
-            }
-        }
-        if (bestMatch != null) {
-            cacheMethod(key, bestMatch);
-        } else {
-            // didn't find a match
-            LOG.trace("No match found.");
-        }
-        return bestMatch;
-    }
-
-    /**
-     * Gets the number of steps required needed to turn the source class into the destination class. This represents the number of steps in the object hierarchy
-     * graph.
-     *
-     * @param srcClass  The source class.
-     * @param destClass The destination class.
-     * @return The cost of transforming an object.
-     */
-    private static float getObjectTransformationCost(Class<?> srcClass, final Class<?> destClass) {
-        float cost = 0.0f;
-        while (srcClass != null && !destClass.equals(srcClass)) {
-            if (destClass.isPrimitive()) {
-                final Class<?> destClassWrapperClazz = ClassUtils.wrapperToPrimitive(destClass);
-                if (destClassWrapperClazz != null && destClassWrapperClazz.equals(srcClass)) {
-                    cost += 0.25f;
-                    break;
-                }
-            }
-            final Class<?> cls = srcClass;
-            if (destClass.isInterface() && ClassUtils.isAssignable(cls, destClass)) {
-                // slight penalty for interface match.
-                // we still want an exact match to override an interface match, but
-                // an interface match should override anything where we have to get a
-                // superclass.
-                cost += 0.25f;
-                break;
-            }
-            cost++;
-            srcClass = srcClass.getSuperclass();
-        }
-
-        /*
-         * If the destination class is null, we've traveled all the way up to an Object match. We'll penalize this by adding 1.5 to the cost.
-         */
-        if (srcClass == null) {
-            cost += 1.5f;
-        }
-
-        return cost;
-    }
-
-    /**
-     * Gets the sum of the object transformation cost for each class in the source argument list.
-     *
-     * @param srcArgs  The source arguments.
-     * @param destArgs The destination arguments.
-     * @return The total transformation cost.
-     */
-    private static float getTotalTransformationCost(final Class<?>[] srcArgs, final Class<?>[] destArgs) {
-        float totalCost = 0.0f;
-        for (int i = 0; i < srcArgs.length; i++) {
-            Class<?> srcClass, destClass;
-            srcClass = srcArgs[i];
-            destClass = destArgs[i];
-            totalCost += getObjectTransformationCost(srcClass, destClass);
-        }
-        return totalCost;
+        });
     }
 
     /**
@@ -496,9 +341,9 @@ private static float getTotalTransformationCost(final Class<?>[] srcArgs, final
      * @since 1.8.0
      */
     public static synchronized void setCacheMethods(final boolean cacheMethods) {
-        CACHE_METHODS = cacheMethods;
-        if (!CACHE_METHODS) {
-            clearCache();
+        CACHE_ENABLED = cacheMethods;
+        if (!CACHE_ENABLED) {
+            CACHE.clear();
         }
     }