[jboss-user] [JBoss AOP Development] New message: "Re: ClassPool Refactoring"

[Flavia Rainone]

User development,

A new message was posted in the thread "ClassPool Refactoring":

http://community.jboss.org/message/522355#522355

Author  : Flavia Rainone
Profile : http://community.jboss.org/people/flavia.rainone@jboss.com

Message:
--------------------------------------------------------------
> mailto:kabir.khan at jboss.com wrote:
>  
> > mailto:flavia.rainone at jboss.com wrote:
> > - why is it that calling get on the default domain returns a different version of the same class? This needs further investigation because it is clearly a bug
> Yes, this definitely needs fixing. But I thought we had tests for this already? e.g. this from SimpleDelegatingClassPoolTestCase. I added a few extra checks as indicated to make double sure
>  
>    *public* *void* testOnePoolPerClassLoadedByA() *throws* Exception
>    {
>       ClassPoolDomain domain = createClassPoolDomain("SIMPLE", null, *false*);
>       ClassPool poolA = createDelegatingClassPool(domain, JAR_A_URL);
>       ClassPool poolB = createDelegatingClassPool(domain, JAR_B_URL);
>       
>       //The first time we access the pool it will create the classes, second time will use the cache
>       accessOnePoolPerClassLoadedByA(poolA, poolB);
>       accessOnePoolPerClassLoadedByA(poolA, poolB);
>    }
>    
>    private  *void* accessOnePoolPerClassLoadedByA(ClassPool poolA, ClassPool poolB) *throws* Exception
>    {
>       CtClass a = poolA.get(CLASS_A);
>       assertEquals(a, poolA.get(CLASS_A));//Added
>       assertEquals(a, poolB.get(CLASS_A));//Added
>       CtClass b = poolA.get(CLASS_B);
>       assertEquals(b, poolA.get(CLASS_B));//Added
>       assertEquals(b, poolB.get(CLASS_B));//Added
>  
>       assertNotSame(a.getClassPool(), b.getClassPool());
>       assertEquals(poolA, a.getClassPool());
>       assertEquals(poolB, b.getClassPool());
>    }
>  
> 
> 
> 
    For some reason yet to be found, those tests do not replicate what is happening in the tests in AS. I'll have to do more debugging to find out exactly what is going on and then add any tests to jboss-classpool if needed.
 
 
> mailto:kabir.khan at jboss.com wrote:
> > mailto:flavia.rainone at jboss.com wrote:
> >  
> > - why wasn't the classes cache being used by BaseClassPool.get before? Any reason for this? This has been copied from previous versions, I think that it definetly causes some overhead (the above should work, but it would be faster if the classpool finds out it has already created the class and returns the same class instead of delegating to the domain first).
> I can't see anything in BaseClassPool.get() or get0() regarding this? I think you mean DelegatingClassPool.get0()? I don't see the ClassPool.classes cache being used there either, so maybe I am looking in the wrong place. If I am in the right place, it might be an idea to try to load it locally in the initiating classpool first before hitting the domain if the cachedLookups == null.
>  
>    //TODO KABIR was synchronized - I don't see why apart from that the standard javassist.ClassPool implementation was synchronized?
>    *public* *final* CtClass get0(String classname, *boolean* useCache) *throws* NotFoundException
>    {
>       //KABIR Made final just to make sure that cached lookups are only handled in one place.
>       
>       *if* (cachedLookups != *null*)
>       {
>          CtClass cachedLookup = cachedLookups.get(classname, domain.getModCount());
>          *if* (cachedLookup != *null*)
>          {
>             logger.trace(classname + " was found in the cache of " + *this*);
>             *return* cachedLookup;
>          }
>       }
>       *else*
>       {
>          CtClass cached = getCachedLocally(classname);
>          *if* (cached != *null*)
>             *return* cached;
>       }
>       
>      ...
>    }
> 
That's the point  I'm saying that, IMO, the first thing that BaseClassPool.get should do is looking in the cache. Currently, it doesn't. That has been done only locally in my machine, and fixed the failing tests. I haven't comitted anything because none of this is 100% validated yet.
 
> mailto:kabir.khan at jboss.com wrote:
> > mailto:flavia.rainone at jboss.com wrote:
> > - if the cache starts being used as a first step of BaseClassPool.get, as it is in my local fix (not committed yet), is there need for a second level cache as Kabir wrote?
> Yes. It caches all CtClasses looked up by that classpool, and they might come from any classpool in the domain. The cacheLookups of all the classpools in the domain are invalidated when a classpool is added/removed to the domain or its parent domain. Basically, it means that if we look something up in a classpool, we don't need to do all the work in the domain.
  I see now. This looks nice . Testing the cache in AS is next in my list.


--------------------------------------------------------------

To reply to this message visit the message page: http://community.jboss.org/message/522355#522355