6565585: Remove critical section in Method.invoke, Constructor.newInstance, Field.getFieldAccessor improving performance
Reviewed-by: alanb, dholmes, briangoetz
--- a/jdk/src/share/classes/java/lang/reflect/AccessibleObject.java Mon Apr 04 19:32:56 2011 +0100
+++ b/jdk/src/share/classes/java/lang/reflect/AccessibleObject.java Mon Apr 04 11:55:05 2011 -0700
@@ -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 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
+ }
}
--- a/jdk/src/share/classes/java/lang/reflect/Constructor.java Mon Apr 04 19:32:56 2011 +0100
+++ b/jdk/src/share/classes/java/lang/reflect/Constructor.java Mon Apr 04 11:55:05 2011 -0700
@@ -74,14 +74,6 @@
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 @@
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 @@
// 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
--- a/jdk/src/share/classes/java/lang/reflect/Field.java Mon Apr 04 19:32:56 2011 +0100
+++ b/jdk/src/share/classes/java/lang/reflect/Field.java Mon Apr 04 11:55:05 2011 -0700
@@ -79,11 +79,6 @@
// 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 @@
tmp = reflectionFactory.newFieldAccessor(this, overrideFinalCheck);
setFieldAccessor(tmp, overrideFinalCheck);
}
+
return tmp;
}
@@ -983,21 +979,8 @@
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);
}
}
}
--- a/jdk/src/share/classes/java/lang/reflect/Method.java Mon Apr 04 19:32:56 2011 +0100
+++ b/jdk/src/share/classes/java/lang/reflect/Method.java Mon Apr 04 11:55:05 2011 -0700
@@ -83,11 +83,6 @@
// 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 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 @@
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 @@
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 @@
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 @@
((Class)exceptions[k]).getName():
exceptions[k].toString());
if (k < (exceptions.length - 1))
- sb.append(",");
+ sb.append(',');
}
}
return sb.toString();
@@ -591,26 +587,15 @@
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 @@
// (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