JBAOP-244 - Optimize away reflection

[kabirkhan]

Can you please point me towards the class you mention here?

http://jira.jboss.com/jira/browse/JBAOP-244

[adrian.brock]

I refactored it to this class:
org.jboss.aop.microcontainer.integration.AOPDependencyBuilder

e.g. Bad code (creates reflection objects repeatedly)

 private void getMethodAnnotationDependencies(Class clazz, MetaDataContext metaDataContext, HashSet dependencies) throws Exception
 {
 Method[] methods = clazz.getMethods();
...
or
 private String getAnnotationStringAttribute(Object annotation, Class annotationType, String attribute)
 {
 try
 {
 Method m = annotation.getClass().getMethod(attribute, (Class[]) null);

This should use the ClassInfo to cache the reflection.

[kabirkhan]

I am in the process of adding this, but have noticed one thing:

 IntrospectionTypeInfoFactory factory = new IntrospectionTypeInfoFactory();
 ClassInfo classInfo = (ClassInfo)factory.getTypeInfo(annotation.getClass());

 classInfo.getDeclaredMethods();

 MethodInfo methodInfo = classInfo.getDeclaredMethod(attribute, (TypeInfo[])null);
 return (String)methodInfo.invoke(annotation, (Object[])null);

Unless I "warm" the classinfo with the call to getDeclaredMethods(), the call to getDeclaredMethod() will always return null;

[adrian.brock]

Should be fixed now.

You know there is a MethodInfo.NO_PARAMS_TYPES instead of passing null?
That makes the code more readable.

[kabirkhan]

Should be fixed now.
You know there is a MethodInfo.NO_PARAMS_TYPES instead of passing null?

I do now :-)

[kabirkhan]

I was going to use the ClassInfo to get hold of the annotations for the classes/methods etc. but ClassInfo.getAnnotations() returns a zero length array of AnnotationValue even for an annotated class. This does not seem to have been implemented yet?

Is this to avoid coupling the MC to JDK 5 features until it works with JBoss Retro? I'm still a bit hazy on the JBoss Retro stuff...

[adrian.brock]

Is this to avoid coupling the MC to JDK 5 features until it works with JBoss Retro? I'm still a bit hazy on the JBoss Retro stuff...

Not really. It is waiting for you (or me, or somebody else) to come up with
a way of sharing code between the 3 current implementations:
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=77746

I don't see the point of wasting memory and storing this info 3+ times.

[kabirkhan]

I've started implementing the annotation stuff in the ClassInfo's as discussed in Neuchatel. One further question:

Do we have anything in place to create the appropriate Value subclass? I've done a search and cannot find anything calling the constructors for StringValueImpl & his friends. I'm happy to create this, but obviously don't want to reinvent the wheel

[kabirkhan]

I've got the annotations optimized away and have added two new tests to test the annotations work:

org.jboss.test.classinfo.test.IntrospectionAnnotatedClassInfoTestCase
org.jboss.test.classinfo.test.JavassistAnnotatedClassInfoTestCase

These are set up and work similarly to the main ClassInfo tests, however for the Introspection flavour one I had to override the getDelegate() method and disable security, since Class.getAnnotations() internally calls Class.getDeclaredMethods() which requires the accessDeclaredMembers permission.

If security is enabled

java.security.AccessControlException: access denied (java.lang.RuntimePermission accessDeclaredMembers)
 at java.security.AccessControlContext.checkPermission(AccessControlContext.java:264)
 at java.security.AccessController.checkPermission(AccessController.java:427)
 at java.lang.SecurityManager.checkPermission(SecurityManager.java:532)
 at java.lang.SecurityManager.checkMemberAccess(SecurityManager.java:1662)
 at java.lang.Class.checkMemberAccess(Class.java:2125)
 at java.lang.Class.getDeclaredMethods(Class.java:1762)
 at sun.reflect.annotation.AnnotationType.<init>(AnnotationType.java:81)
 at sun.reflect.annotation.AnnotationType.getInstance(AnnotationType.java:64)
 at sun.reflect.annotation.AnnotationParser.parseAnnotation(AnnotationParser.java:202)
 at sun.reflect.annotation.AnnotationParser.parseAnnotations2(AnnotationParser.java:69)
 at sun.reflect.annotation.AnnotationParser.parseAnnotations(AnnotationParser.java:52)
 at java.lang.Class.initAnnotationsIfNecessary(Class.java:2998)
 at java.lang.Class.getAnnotations(Class.java:2980)
 at org.jboss.reflect.plugins.introspection.IntrospectionTypeInfoFactoryImpl.getAnnotations(IntrospectionTypeInfoFactoryImpl.java:97)
 at org.jboss.reflect.plugins.InheritableAnnotationHolder.getAnnotations(InheritableAnnotationHolder.java:105)
 at org.jboss.test.classinfo.test.AnnotatedClassInfoTest.checkAnnotations(AnnotatedClassInfoTest.java:254)
 at org.jboss.test.classinfo.test.AnnotatedClassInfoTest.testClassAnnotations(AnnotatedClassInfoTest.java:143)

[kabirkhan]

For the issue mentioned above, I added http://jira.jboss.com/jira/browse/JBAOP-266 to the FIXME holder task

[kabirkhan]

Using PrivilegedActions when reading solves the error

[adrian.brock]

I've started implementing the annotation stuff in the ClassInfo's as discussed in Neuchatel. One further question:

Do we have anything in place to create the appropriate Value subclass? I've done a search and cannot find anything calling the constructors for StringValueImpl & his friends. I'm happy to create this, but obviously don't want to reinvent the wheel

IIRC, currently, the annotations stuff doesn't use the property editors
for the values.
It should, via the StringValueMetaData.