6565585: Remove critical section in Method.invoke, Constructor.newInstance,...

6565585: Remove critical section in Method.invoke, Constructor.newInstance, Field.getFieldAccessor improving performance
Reviewed-by: alanb, dholmes, briangoetz

src/share/classes/java/lang/reflect/AccessibleObject.java

@@ -26,6 +26,7 @@
 package java.lang.reflect;
 
 import java.security.AccessController;
+import sun.reflect.Reflection;
 import sun.reflect.ReflectionFactory;
 import java.lang.annotation.Annotation;
 
@@ -201,4 +202,73 @@ public class AccessibleObject implements AnnotatedElement {
     public Annotation[] getDeclaredAnnotations()  {
         throw new AssertionError("All subclasses should override this method");
     }
+
+
+    // Shared access checking logic.
+
+    // For non-public members or members in package-private classes,
+    // it is necessary to perform somewhat expensive security checks.
+    // If the security check succeeds for a given class, it will
+    // always succeed (it is not affected by the granting or revoking
+    // of permissions); we speed up the check in the common case by
+    // remembering the last Class for which the check succeeded.
+    //
+    // The simple security check for Constructor is to see if
+    // the caller has already been seen, verified, and cached.
+    // (See also Class.newInstance(), which uses a similar method.)
+    //
+    // A more complicated security check cache is needed for Method and Field
+    // The cache can be either null (empty cache), a 2-array of {caller,target},
+    // or a caller (with target implicitly equal to this.clazz).
+    // In the 2-array case, the target is always different from the clazz.
+    volatile Object securityCheckCache;
+
+    void checkAccess(Class<?> caller, Class<?> clazz, Object obj, int modifiers)
+        throws IllegalAccessException
+    {
+        if (caller == clazz) {  // quick check
+            return;             // ACCESS IS OK
+        }
+        Object cache = securityCheckCache;  // read volatile
+        Class<?> targetClass = clazz;
+        if (obj != null
+            && Modifier.isProtected(modifiers)
+            && ((targetClass = obj.getClass()) != clazz)) {
+            // Must match a 2-list of { caller, targetClass }.
+            if (cache instanceof Class[]) {
+                Class<?>[] cache2 = (Class<?>[]) cache;
+                if (cache2[1] == targetClass &&
+                    cache2[0] == caller) {
+                    return;     // ACCESS IS OK
+                }
+                // (Test cache[1] first since range check for [1]
+                // subsumes range check for [0].)
+            }
+        } else if (cache == caller) {
+            // Non-protected case (or obj.class == this.clazz).
+            return;             // ACCESS IS OK
+        }
+
+        // If no return, fall through to the slow path.
+        slowCheckMemberAccess(caller, clazz, obj, modifiers, targetClass);
+    }
+
+    // Keep all this slow stuff out of line:
+    void slowCheckMemberAccess(Class<?> caller, Class<?> clazz, Object obj, int modifiers,
+                               Class<?> targetClass)
+        throws IllegalAccessException
+    {
+        Reflection.ensureMemberAccess(caller, clazz, obj, modifiers);
+
+        // Success: Update the cache.
+        Object cache = ((targetClass == clazz)
+                        ? caller
+                        : new Class<?>[] { caller, targetClass });
+
+        // Note:  The two cache elements are not volatile,
+        // but they are effectively final.  The Java memory model
+        // guarantees that the initializing stores for the cache
+        // elements will occur before the volatile write.
+        securityCheckCache = cache;         // write volatile
+    }
 }

src/share/classes/java/lang/reflect/Constructor.java

@@ -74,14 +74,6 @@ public final
     private byte[]              annotations;
     private byte[]              parameterAnnotations;
 
-    // For non-public members or members in package-private classes,
-    // it is necessary to perform somewhat expensive security checks.
-    // If the security check succeeds for a given class, it will
-    // always succeed (it is not affected by the granting or revoking
-    // of permissions); we speed up the check in the common case by
-    // remembering the last Class for which the check succeeded.
-    private volatile Class<?> securityCheckCache;
-
     // Generics infrastructure
     // Accessor for factory
     private GenericsFactory getFactory() {
@@ -518,16 +510,17 @@ public final
         if (!override) {
             if (!Reflection.quickCheckMemberAccess(clazz, modifiers)) {
                 Class<?> caller = Reflection.getCallerClass(2);
-                if (securityCheckCache != caller) {
-                    Reflection.ensureMemberAccess(caller, clazz, null, modifiers);
-                    securityCheckCache = caller;
-                }
+
+                checkAccess(caller, clazz, null, modifiers);
             }
         }
         if ((clazz.getModifiers() & Modifier.ENUM) != 0)
             throw new IllegalArgumentException("Cannot reflectively create enum objects");
-        if (constructorAccessor == null) acquireConstructorAccessor();
-        return (T) constructorAccessor.newInstance(initargs);
+        ConstructorAccessor ca = constructorAccessor;   // read volatile
+        if (ca == null) {
+            ca = acquireConstructorAccessor();
+        }
+        return (T) ca.newInstance(initargs);
     }
 
     /**
@@ -560,18 +553,20 @@ public final
     // ConstructorAccessor for a given Constructor. However, avoiding
     // synchronization will probably make the implementation more
     // scalable.
-    private void acquireConstructorAccessor() {
+    private ConstructorAccessor acquireConstructorAccessor() {
         // First check to see if one has been created yet, and take it
         // if so.
         ConstructorAccessor tmp = null;
         if (root != null) tmp = root.getConstructorAccessor();
         if (tmp != null) {
             constructorAccessor = tmp;
-            return;
+        } else {
+            // Otherwise fabricate one and propagate it up to the root
+            tmp = reflectionFactory.newConstructorAccessor(this);
+            setConstructorAccessor(tmp);
         }
-        // Otherwise fabricate one and propagate it up to the root
-        tmp = reflectionFactory.newConstructorAccessor(this);
-        setConstructorAccessor(tmp);
+
+        return tmp;
     }
 
     // Returns ConstructorAccessor for this Constructor object, not

src/share/classes/java/lang/reflect/Field.java

@@ -79,11 +79,6 @@ class Field extends AccessibleObject implements Member {
     // potentially many Field objects pointing to it.)
     private Field               root;
 
-    // More complicated security check cache needed here than for
-    // Class.newInstance() and Constructor.newInstance()
-    private Class<?> securityCheckCache;
-    private Class<?> securityCheckTargetClassCache;
-
     // Generics infrastructure
 
     private String getGenericSignature() {return signature;}
@@ -954,6 +949,7 @@ class Field extends AccessibleObject implements Member {
             tmp = reflectionFactory.newFieldAccessor(this, overrideFinalCheck);
             setFieldAccessor(tmp, overrideFinalCheck);
         }
+
         return tmp;
     }
 
@@ -983,21 +979,8 @@ class Field extends AccessibleObject implements Member {
         if (!override) {
             if (!Reflection.quickCheckMemberAccess(clazz, modifiers)) {
                 Class<?> caller = Reflection.getCallerClass(4);
-                Class<?> targetClass = ((obj == null || !Modifier.isProtected(modifiers))
-                                        ? clazz
-                                        : obj.getClass());
 
-                synchronized (this) {
-                    if ((securityCheckCache == caller)
-                            && (securityCheckTargetClassCache == targetClass)) {
-                        return;
-                    }
-                }
-                Reflection.ensureMemberAccess(caller, clazz, obj, modifiers);
-                synchronized (this) {
-                    securityCheckCache = caller;
-                    securityCheckTargetClassCache = targetClass;
-                }
+                checkAccess(caller, clazz, obj, modifiers);
             }
         }
     }

src/share/classes/java/lang/reflect/Method.java

@@ -83,11 +83,6 @@ public final
     // potentially many Method objects pointing to it.)
     private Method              root;
 
-    // More complicated security check cache needed here than for
-    // Class.newInstance() and Constructor.newInstance()
-    private Class<?> securityCheckCache;
-    private Class<?> securityCheckTargetClassCache;
-
    // Generics infrastructure
 
     private String getGenericSignature() {return signature;}
@@ -402,28 +397,28 @@ public final
      */
     public String toString() {
         try {
-            StringBuffer sb = new StringBuffer();
+            StringBuilder sb = new StringBuilder();
             int mod = getModifiers() & Modifier.methodModifiers();
             if (mod != 0) {
-                sb.append(Modifier.toString(mod) + " ");
+                sb.append(Modifier.toString(mod)).append(' ');
             }
-            sb.append(Field.getTypeName(getReturnType()) + " ");
-            sb.append(Field.getTypeName(getDeclaringClass()) + ".");
-            sb.append(getName() + "(");
+            sb.append(Field.getTypeName(getReturnType())).append(' ');
+            sb.append(Field.getTypeName(getDeclaringClass())).append('.');
+            sb.append(getName()).append('(');
             Class<?>[] params = parameterTypes; // avoid clone
             for (int j = 0; j < params.length; j++) {
                 sb.append(Field.getTypeName(params[j]));
                 if (j < (params.length - 1))
-                    sb.append(",");
+                    sb.append(',');
             }
-            sb.append(")");
+            sb.append(')');
             Class<?>[] exceptions = exceptionTypes; // avoid clone
             if (exceptions.length > 0) {
                 sb.append(" throws ");
                 for (int k = 0; k < exceptions.length; k++) {
                     sb.append(exceptions[k].getName());
                     if (k < (exceptions.length - 1))
-                        sb.append(",");
+                        sb.append(',');
                 }
             }
             return sb.toString();
@@ -475,15 +470,15 @@ public final
             StringBuilder sb = new StringBuilder();
             int mod = getModifiers() & Modifier.methodModifiers();
             if (mod != 0) {
-                sb.append(Modifier.toString(mod) + " ");
+                sb.append(Modifier.toString(mod)).append(' ');
             }
             TypeVariable<?>[] typeparms = getTypeParameters();
             if (typeparms.length > 0) {
                 boolean first = true;
-                sb.append("<");
+                sb.append('<');
                 for(TypeVariable<?> typeparm: typeparms) {
                     if (!first)
-                        sb.append(",");
+                        sb.append(',');
                     // Class objects can't occur here; no need to test
                     // and call Class.getName().
                     sb.append(typeparm.toString());
@@ -494,10 +489,11 @@ public final
 
             Type genRetType = getGenericReturnType();
             sb.append( ((genRetType instanceof Class<?>)?
-                        Field.getTypeName((Class<?>)genRetType):genRetType.toString())  + " ");
+                        Field.getTypeName((Class<?>)genRetType):genRetType.toString()))
+                    .append(' ');
 
-            sb.append(Field.getTypeName(getDeclaringClass()) + ".");
-            sb.append(getName() + "(");
+            sb.append(Field.getTypeName(getDeclaringClass())).append('.');
+            sb.append(getName()).append('(');
             Type[] params = getGenericParameterTypes();
             for (int j = 0; j < params.length; j++) {
                 String param = (params[j] instanceof Class)?
@@ -507,9 +503,9 @@ public final
                     param = param.replaceFirst("\\[\\]$", "...");
                 sb.append(param);
                 if (j < (params.length - 1))
-                    sb.append(",");
+                    sb.append(',');
             }
-            sb.append(")");
+            sb.append(')');
             Type[] exceptions = getGenericExceptionTypes();
             if (exceptions.length > 0) {
                 sb.append(" throws ");
@@ -518,7 +514,7 @@ public final
                               ((Class)exceptions[k]).getName():
                               exceptions[k].toString());
                     if (k < (exceptions.length - 1))
-                        sb.append(",");
+                        sb.append(',');
                 }
             }
             return sb.toString();
@@ -591,26 +587,15 @@ public final
         if (!override) {
             if (!Reflection.quickCheckMemberAccess(clazz, modifiers)) {
                 Class<?> caller = Reflection.getCallerClass(1);
-                Class<?> targetClass = ((obj == null || !Modifier.isProtected(modifiers))
-                                        ? clazz
-                                        : obj.getClass());
-
-                boolean cached;
-                synchronized (this) {
-                    cached = (securityCheckCache == caller)
-                            && (securityCheckTargetClassCache == targetClass);
-                }
-                if (!cached) {
-                    Reflection.ensureMemberAccess(caller, clazz, obj, modifiers);
-                    synchronized (this) {
-                        securityCheckCache = caller;
-                        securityCheckTargetClassCache = targetClass;
-                    }
-                }
+
+                checkAccess(caller, clazz, obj, modifiers);
             }
         }
-        if (methodAccessor == null) acquireMethodAccessor();
-        return methodAccessor.invoke(obj, args);
+        MethodAccessor ma = methodAccessor;             // read volatile
+        if (ma == null) {
+            ma = acquireMethodAccessor();
+        }
+        return ma.invoke(obj, args);
     }
 
     /**
@@ -654,18 +639,20 @@ public final
     // (though not efficient) to generate more than one MethodAccessor
     // for a given Method. However, avoiding synchronization will
     // probably make the implementation more scalable.
-    private void acquireMethodAccessor() {
+    private MethodAccessor acquireMethodAccessor() {
         // First check to see if one has been created yet, and take it
         // if so
         MethodAccessor tmp = null;
         if (root != null) tmp = root.getMethodAccessor();
         if (tmp != null) {
             methodAccessor = tmp;
-            return;
+        } else {
+            // Otherwise fabricate one and propagate it up to the root
+            tmp = reflectionFactory.newMethodAccessor(this);
+            setMethodAccessor(tmp);
         }
-        // Otherwise fabricate one and propagate it up to the root
-        tmp = reflectionFactory.newMethodAccessor(this);
-        setMethodAccessor(tmp);
+
+        return tmp;
     }
 
     // Returns MethodAccessor for this Method object, not looking up