This content has been marked as final.
Show 24 replies
-
15. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
alesj Nov 20, 2008 9:31 AM (in response to starksm64)"adrian@jboss.org" 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) -
16. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
alesj Nov 20, 2008 9:44 AM (in response to starksm64)"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; }
-
17. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
adrian.brock Nov 20, 2008 9:45 AM (in response to starksm64)"alesj" wrote:
"adrian@jboss.org" 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. -
18. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
alesj Nov 20, 2008 10:00 AM (in response to starksm64)"adrian@jboss.org" wrote:
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 thistestGenericSuperClassString(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)
:-) -
19. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
adrian.brock Nov 20, 2008 10:18 AM (in response to starksm64)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. -
20. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
adrian.brock Nov 20, 2008 10:33 AM (in response to starksm64)"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. ;-) -
21. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
adrian.brock Nov 20, 2008 10:36 AM (in response to starksm64)"adrian@jboss.org" wrote:
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. -
22. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
alesj Nov 20, 2008 11:05 AM (in response to starksm64)"adrian@jboss.org" 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:"adrian@jboss.org" wrote:
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. :-) -
23. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
adrian.brock Nov 20, 2008 12:21 PM (in response to starksm64)"alesj" wrote:
"adrian@jboss.org" 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:"adrian@jboss.org" wrote:
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. -
24. Re: UnwrapValueUnitTestCase.testCollectionUnwrap failure on
alesj Nov 21, 2008 3:58 AM (in response to starksm64)"adrian@jboss.org" wrote:
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. :-)