[alesj]

Clearly, the caching in AbstractBeanInfoFactory shouldn't be using
classInfo.getName() in its caching.

What about if I just change that the key is ClassInfo instead of String?
(I've done that locally and all the tests pass)

[alesj]

"alesj" wrote:

What about if I just change that the key is ClassInfo instead of String?
(I've done that locally and all the tests pass)

Actually I think I can drop the whole ClassLoader as key,
since ClassInfo is per ClassLoader if I'm not mistaken.

Index: src/main/java/org/jboss/beans/info/plugins/AbstractBeanInfoFactory.java
===================================================================
--- src/main/java/org/jboss/beans/info/plugins/AbstractBeanInfoFactory.java (revision 79796)
+++ src/main/java/org/jboss/beans/info/plugins/AbstractBeanInfoFactory.java (working copy)
@@ -55,7 +55,7 @@
 public class AbstractBeanInfoFactory implements BeanInfoFactory
 {
 /** The cache */
- protected Map<ClassLoader, Map<String, Map<BeanAccessMode, BeanInfo>>> cache = new WeakHashMap<ClassLoader, Map<String, Map<BeanAccessMode, BeanInfo>>>();
+ protected Map<ClassInfo, Map<BeanAccessMode, BeanInfo>> cache = new WeakHashMap<ClassInfo, Map<BeanAccessMode, BeanInfo>>();

 protected static boolean isGetter(MethodInfo minfo)
 {
@@ -133,20 +133,13 @@

 synchronized (cache)
 {
- ClassLoader cl = classAdapter.getClassLoader();
 ClassInfo classInfo = classAdapter.getClassInfo();
- String className = classInfo.getName();
- Map<String, Map<BeanAccessMode, BeanInfo>> map = cache.get(cl);
- Map<BeanAccessMode, BeanInfo> modeMap = null;
- if (map != null)
+ Map<BeanAccessMode, BeanInfo> modeMap = cache.get(classInfo);
+ if (modeMap != null)
 {
- modeMap = map.get(className);
- if (modeMap != null)
- {
- BeanInfo info = modeMap.get(accessMode);
- if (info != null)
- return info;
- }
+ BeanInfo info = modeMap.get(accessMode);
+ if (info != null)
+ return info;
 }

 Set<ConstructorInfo> constructors = getConstructors(classInfo);
@@ -159,16 +152,12 @@
 Set<EventInfo> events = getEvents(classInfo);

 BeanInfo result = createBeanInfo(classAdapter, accessMode, properties, constructors, methods, events);
- if (map == null)
- {
- map = new WeakValueHashMap<String, Map<BeanAccessMode, BeanInfo>>();
- cache.put(cl, map);
- }
 if (modeMap == null)
 {
 modeMap = new WeakValueHashMap<BeanAccessMode, BeanInfo>();
- map.put(className, modeMap);
+ cache.put(classInfo, modeMap);
 }
+
 modeMap.put(accessMode, result);
 return result;
 }

[adrian.brock]

"alesj" wrote:

Clearly, the caching in AbstractBeanInfoFactory shouldn't be using
classInfo.getName() in its caching.

What about if I just change that the key is ClassInfo instead of String?
(I've done that locally and all the tests pass)

But it wouldn't be much of a cache since the ClassInfo's can be pretty transient.

Also I think it will cause a memory leak.

You effectively have:

WeakValueHashMap<ClassInfo, BeanInfo>

but the ClassInfo could itself get a hard reference on the BeanInfo
via one of its "attachments", e.g. the caching of SchemaBindings in jbossxb
against the TypeInfo.

[alesj]

Or if it is going to use that, then ParameterizedClassInfo needs to override getName()
to return the generic name (but I'm not sure what that would break?).

This

+++ src/main/java/org/jboss/reflect/plugins/introspection/ParameterizedClassInfo.java
@@ -77,6 +77,12 @@
 this.parameterizedType = parameterizedType;
 }

+ @Override
+ public String getName()
+ {
+ return parameterizedType.toString();
+ }

breaks this

 testGenericSuperClassString(org.jboss.test.classinfo.test.IntrospectionGenericClassUnitTestCase)
 testGenericSuperClassEmptyClass(org.jboss.test.classinfo.test.IntrospectionGenericClassUnitTestCase)
 testGenericSuperInterfaceString(org.jboss.test.classinfo.test.IntrospectionGenericClassUnitTestCase)
 testGenericSuperInterfaceEmptyClass(org.jboss.test.classinfo.test.IntrospectionGenericClassUnitTestCase)

:-)

[adrian.brock]

I don't know how to make the test fail, but the following
change to the test shows the problem happening.

Index: src/test/java/org/jboss/test/metatype/values/factory/test/UnwrapValueUnitTestCase.java
===================================================================
--- src/test/java/org/jboss/test/metatype/values/factory/test/UnwrapValueUnitTestCase.java (revision 81358)
+++ src/test/java/org/jboss/test/metatype/values/factory/test/UnwrapValueUnitTestCase.java (working copy)
@@ -35,7 +35,10 @@

 import junit.framework.Test;

+import org.jboss.beans.info.spi.BeanInfo;
+import org.jboss.config.plugins.property.PropertyConfiguration;
 import org.jboss.metatype.api.values.MetaValue;
+import org.jboss.reflect.spi.ClassInfo;
 import org.jboss.test.metatype.values.factory.support.SimpleCompositeInterface;
 import org.jboss.test.metatype.values.factory.support.TestEnum;
 import org.jboss.test.metatype.values.factory.support.TestGeneric;
@@ -103,9 +106,21 @@

 checkSingle(new TestSimpleCompositeInterface("something"), SimpleCompositeInterface.class);
 }
+ PropertyConfiguration config = new PropertyConfiguration();

+ private void printCollection(Type type)
+ {
+ ClassInfo typeInfo = (ClassInfo) config.getTypeInfo(type);
+ System.out.println(typeInfo.getName() + " " + typeInfo.getComponentType());
+ BeanInfo beanInfo = config.getBeanInfo(typeInfo);
+ typeInfo = beanInfo.getClassInfo();
+ System.out.println(typeInfo.getName() + " " + typeInfo.getComponentType());
+ }
+
 public void testCollectionUnwrap() throws Exception
 {
+ printCollection(getType("Integer", Set.class));
+ printCollection(getType("Enum", Set.class));
 Integer i1 = 123;
 Integer i2 = 123;
 checkCollection(new ArrayList<Integer>(), i1, i2);

which produces output (the last line is wrong)

java.util.Set java.lang.Integer
java.util.Set java.lang.Integer
java.util.Set EnumInfoImpl@1df280b{name=org.jboss.test.metatype.values.factory.support.TestEnum}
java.util.Set java.lang.Integer

So it looks like the BeanInfo caching is the problem.

[adrian.brock]

"alesj" wrote:

"alesj" wrote:

What about if I just change that the key is ClassInfo instead of String?
(I've done that locally and all the tests pass)

Actually I think I can drop the whole ClassLoader as key,
since ClassInfo is per ClassLoader if I'm not mistaken.

But you still have the problem that ClassInfos can be transient.

Let's go with that, but we need to do so extra work to see whether all this
caching is (still) effective.

I think in practice it will work for most of the ClassInfos we use to create beans
since the WeakTypeCache will hold a reference to the TypeInfo until the bean's class
is GCed.

But for more transient bean classes, e.g. those used only in the managed object layer
it might not be as good?

If only there was a combination Weak/SoftReference mechanism
which let you do SoftReference behaviour until the classloader gets GCed
then it turns into WeakReference behaviour. ;-)

[adrian.brock]

But for more transient bean classes, e.g. those used only in the managed object layer
it might not be as good?

Also for the generic types, it will depend how the JDK's reflection framework
holds references to those objects.

[alesj]

Let's go with that, but we need to do so extra work to see whether all this
caching is (still) effective.

But won't this interfere with this:

Also I think it will cause a memory leak.

You effectively have:

WeakValueHashMap<ClassInfo, BeanInfo>

but the ClassInfo could itself get a hard reference on the BeanInfo
via one of its "attachments", e.g. the caching of SchemaBindings in jbossxb
against the TypeInfo.

If not, I'll change it + add some caching tests. :-)

[adrian.brock]

"alesj" wrote:

Let's go with that, but we need to do so extra work to see whether all this
caching is (still) effective.

But won't this interfere with this:

Also I think it will cause a memory leak.

You effectively have:

WeakValueHashMap<ClassInfo, BeanInfo>

but the ClassInfo could itself get a hard reference on the BeanInfo
via one of its "attachments", e.g. the caching of SchemaBindings in jbossxb
against the TypeInfo.

I assume you're using a WeakHashMap<ClassInfo, Map>
not a WeakValueHashMap as originally proposed?
The ClassInfo reference is now weak.

If not, I'll change it + add some caching tests. :-)

Test the contract not the implemenation details. ;-)
Otherwise, you'll only have to rewrite the tests when you change the caching alogrithm
or some other detail in future - which kind of makes them pointless.

See my earlier printCollection code. This tests the contract and would be
a valid test with or without caching.
Without caching, it would just be that the Bean/ClassInfo instances
are different == (but equivalent .equals) at each stage.

[alesj]

I assume you're using a WeakHashMap<ClassInfo, Map>
not a WeakValueHashMap as originally proposed?

This was already originally proposed. ;-)

"alesj" wrote:

- protected Map<ClassLoader, Map<String, Map<BeanAccessMode, BeanInfo>>> cache = new WeakHashMap<ClassLoader, Map<String, Map<BeanAccessMode, BeanInfo>>>();
+ protected Map<ClassInfo, Map<BeanAccessMode, BeanInfo>> cache = new WeakHashMap<ClassInfo, Map<BeanAccessMode, BeanInfo>>();

But it was more by luck then intention. :-)