enhance: improve the performance of ObjFactory (#491)

* add jmh to benchmark

* enhance: cache class and method to avoid reading each time

* add comments for new methods in ObjFactory

* enhance: create the exception once and use many times

Author: hellyguo

pom.xml

@@ -381,6 +381,18 @@
                 </exclusion>
             </exclusions>
         </dependency>
+        <dependency>
+            <groupId>org.openjdk.jmh</groupId>
+            <artifactId>jmh-core</artifactId>
+            <version>1.21</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.openjdk.jmh</groupId>
+            <artifactId>jmh-generator-annprocess</artifactId>
+            <version>1.21</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

src/main/java/org/owasp/esapi/util/ObjFactory.java

@@ -13,6 +13,7 @@
 
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * A generic object factory to create an object of class T. T must be a concrete
@@ -44,6 +45,11 @@
  */
 public class ObjFactory {
 
+	private static final int CACHE_INITIAL_CAPACITY = 4096;
+	private static final float CACHE_LOAD_FACTOR = 0.75F;
+	private static final ConcurrentHashMap<String,Class<?>> CLASSES_CACHE = new ConcurrentHashMap<>(CACHE_INITIAL_CAPACITY, CACHE_LOAD_FACTOR);
+	private static final ConcurrentHashMap<String,MethodWrappedInfo> METHODS_CACHE = new ConcurrentHashMap<>(CACHE_INITIAL_CAPACITY, CACHE_LOAD_FACTOR);
+
 	/**
 	 * Create an object based on the <code>className</code> parameter.
 	 * 
@@ -70,20 +76,13 @@ public static <T> T make(String className, String typeName) throws Configuration
 				// No big deal...just use "[unknown?]" for this as it's only for an err msg.
 				typeName = "[unknown?]";	// CHECKME: Any better suggestions?
 			}
-			
-			Class<?> theClass = Class.forName(className);
-
-            try {
-                Method singleton = theClass.getMethod( "getInstance" );
-
-                // If the implementation class contains a getInstance method that is not static, this is an invalid
-                // object configuration and a ConfigurationException will be thrown.
-                if ( !Modifier.isStatic( singleton.getModifiers() ) )
-                {
-                    throw new ConfigurationException( "Class [" + className + "] contains a non-static getInstance method." );
-                }
-                
-                obj = singleton.invoke( null );
+
+			Class<?> theClass = loadClassByStringName(className);
+
+			try {
+				Method singleton = findSingletonCreateMethod(className, theClass);
+
+				obj = singleton.invoke( null );
             } catch (NoSuchMethodException e) {
                 // This is a no-error exception, if this is caught we will continue on assuming the implementation was
                 // not meant to be used as a singleton.
@@ -97,7 +96,7 @@ public static <T> T make(String className, String typeName) throws Configuration
             }
 
 			return (T)obj;		// Eclipse warning here if @SupressWarnings omitted.
-			
+
             // Issue 66 - Removed System.out calls as we are throwing an exception in each of these cases
             // anyhow.
 		} catch( IllegalArgumentException ex ) {
@@ -130,9 +129,98 @@ public static <T> T make(String className, String typeName) throws Configuration
 		}
 		// DISCUSS: Should we also catch ExceptionInInitializerError here? See Google Issue #61 comments.
 	}
-	
+
+	/**
+	 * Load the class in cache, or load by the classloader and cache it
+	 *
+	 * @param className The name of the class to construct. Should be a fully qualified name
+	 * @return The target class
+	 * @throws ClassNotFoundException Failed to load class by the className
+	 */
+	private static Class<?> loadClassByStringName(String className) throws ClassNotFoundException {
+		Class<?> clazz;
+		if (CLASSES_CACHE.containsKey(className)) {
+			clazz = CLASSES_CACHE.get(className);
+		} else {
+			clazz = Class.forName(className);
+			CLASSES_CACHE.putIfAbsent(className, clazz);
+		}
+		return clazz;
+	}
+
 	/**
+	 * Find the method to create a singleton object
+	 *
+	 * @param className The name of the class to construct. Should be a fully qualified name
+	 * @param theClass The class loaded in prior
+	 * @return The method to create a singleton object
+	 * @throws NoSuchMethodException Failed to find the target method
+	 */
+    private static Method findSingletonCreateMethod(String className, Class<?> theClass) throws NoSuchMethodException {
+        MethodWrappedInfo singleton = loadMethodByStringName(className,theClass);
+
+        // If the implementation class contains a getInstance method that is not static, this is an invalid
+        // object configuration and a ConfigurationException will be thrown.
+        if (!singleton.isStaticMethod()) {
+            throw singleton.getNonStaticEx();
+        }
+        return singleton.getMethod();
+    }
+
+	/**
+	 *
+	 * @param className The name of the class to construct. Should be a fully qualified name
+	 * @param theClass The class loaded in prior
+	 * @return Wrapped data, contains the method object and the method is static method or not
+	 * @throws NoSuchMethodException Failed to find the target method
+	 */
+    private static MethodWrappedInfo loadMethodByStringName(String className, Class<?> theClass) throws NoSuchMethodException {
+        String methodName = className + "getInstance";
+        MethodWrappedInfo methodInfo;
+        if (METHODS_CACHE.containsKey(methodName)) {
+            methodInfo = METHODS_CACHE.get(methodName);
+        } else {
+            Method method = theClass.getMethod("getInstance");
+            boolean staticMethod = Modifier.isStatic(method.getModifiers());
+            ConfigurationException nonStaticEx = staticMethod ? null :
+                    new ConfigurationException("Class [" + className + "] contains a non-static getInstance method.");
+            methodInfo = new MethodWrappedInfo(method, staticMethod, nonStaticEx);
+            METHODS_CACHE.putIfAbsent(methodName, methodInfo);
+        }
+        return methodInfo;
+    }
+
+    /**
 	 * Not instantiable
 	 */
 	private ObjFactory() { }
+
+	/**
+	 * Wrapped data, contains the method object and the method is static method or not.<br>
+	 * The goal to store the boolean value in field staticMethod is reduce the check times: check once, use many times.<br>
+     * The goal to store the exception in field nonStaticException is reduce the cost of new Exception(): create once, use many times.
+	 */
+	private static class MethodWrappedInfo {
+		private Method method;
+		private boolean staticMethod;
+		private ConfigurationException nonStaticEx;
+
+		MethodWrappedInfo(Method method, boolean staticMethod, ConfigurationException nonStaticEx) {
+			this.method = method;
+			this.staticMethod = staticMethod;
+			this.nonStaticEx = nonStaticEx;
+		}
+
+		Method getMethod() {
+			return method;
+		}
+
+		boolean isStaticMethod() {
+			return staticMethod;
+		}
+
+        ConfigurationException getNonStaticEx() {
+            return nonStaticEx;
+        }
+    }
 }

src/test/java/org/owasp/esapi/util/ObjFactoryBenchmark.java

@@ -0,0 +1,26 @@
+package org.owasp.esapi.util;
+
+import org.openjdk.jmh.annotations.*;
+import org.openjdk.jmh.infra.Blackhole;
+
+import javax.crypto.NullCipher;
+
+@State(Scope.Benchmark)
+@Fork(value = 3)
+@Warmup(iterations = 5, time = 1)
+@Measurement(iterations = 10, time = 1)
+public class ObjFactoryBenchmark {
+
+    @Benchmark
+    public void benchmark1(Blackhole hole) {
+        NullCipher nullCipher = ObjFactory.make("javax.crypto.NullCipher", "NullCipher");
+        hole.consume(nullCipher);
+    }
+
+    @Benchmark
+    public void benchmark2(Blackhole hole) {
+        String key = ObjFactory.make("java.lang.String", "testMakeNotASubclass");
+        hole.consume(key);
+    }
+
+}