[flavia.rainone]

[e-mail] wrote:

While I am here, I noticed that the point 3) in http://community.jboss.org/message/281717#281717 has not been done. I want to change ClassPoolRepostitory.callback to a list, which would mean changing the API from:

public void setClassPoolRepositoryCallback(ClassPoolRepositoryCallback callback)
public ClassPoolRepositoryCallback getClassPoolRepositoryCallback()
 

to:

public void addClassPoolRepositoryCallback(ClassPoolRepositoryCallback callback)
public void removeClassPoolRepositoryCallback(ClassPoolRepositoryCallback callback)
public Collection<ClassPoolRepositoryCallback> getClassPoolRepositoryCallbacks()

While this is a change in API, I seriously doubt anybody is using this yet, apart from the unreleased AOP stuff Flavia is working on, so I'll make this change unless Ales objects by Monday afternoon :-)

Thanks, Kabir! Doing this and checking for whether it added some overhead was next in my list (I was first dealing with the performance hit, which I considered more urgent).

I'll profile it next week, once I'm back.

[kabirkhan]

I've tried with a cache on the domain, but this breaks the tests using import/exports. I'll try putting this into the pools instead

[kabirkhan]

This has been done against https://jira.jboss.org/jira/browse/JBREFLECT-92. By default the cache is alive for 20 seconds with a 3 second "resolution", but this and the CachePolicy class is overridable with system properties http://anonsvn.jboss.org/repos/jbossas/projects/jboss-classpool/trunk/classpool/src/main/java/org/jboss/classpool/base/CtClassCacheFactory.java

[kabirkhan]

Forgot to mention that I scheduled JBREFLECT-92 for a future release since alpha1 is released and alpha2 will probably be a rename of the beta? In other words I did not schedule it against the beta so it would erroneously get included in alpha2 somehow. Once JIRA is updated for the release, this issue should be moved to the right one

[alesj]

but this and the CachePolicy class is overridable with system properties http://anonsvn.jboss.org/repos/jbossas/projects/jboss-classpool/trunk/classpool/src/main/java/org/jboss/classpool/base/CtClassCacheFactory.java

By default there should be no cache. Is this the case or not?

[kabirkhan]

alesj wrote:

but this and the CachePolicy class is overridable with system properties http://anonsvn.jboss.org/repos/jbossas/projects/jboss-classpool/trunk/classpool/src/main/java/org/jboss/classpool/base/CtClassCacheFactory.java

By default there should be no cache. Is this the case or not?

There is a cache by default at the moment. I'll turn it off, and have the tests run with and without caching

[kabirkhan]

[e-mail] wrote:

There is a cache by default at the moment. I'll turn it off, and have the tests run with and without caching

Done

[flavia.rainone]

I've been testing the trunk version of jboss classpool (with the cache disabled) with AS trunk, and the AOP tests are not passing.

I discovered that the classes are not being woven because the weaving proccess throws a CannotCompileException, indicating that wrapped methods do not exist. The exception is thrown at the moment that code that invokes the wrapped method is generated (dispatch method).

The trigger of this exception is the classpool, that is returning a new, fresh, CtClass instance of the class being woven. Instead, the classpool should return the modified CtClass instance, the one that contains the wrapped method renamed. I'll continue investigating to find out why are the classpools behaving this way.

[flavia.rainone]

A quick update on this: for some reason, the behavior of ClassPool.get method is no longer consistent with ScopedClassPool.getLocally method.

The first time AOP asks for a class, during transformation, it knows that the class should be generated by the exact same classpool that corresponds to the classloader that loaded the class, so it uses getLocally.

The second time AOP asks for the class is during the same transformation (these are actually indirect calls made from inside javassist, during call weaving, for example), the class is retrieved through get. Currently, the classpool delegates the retrieval of the class to the default domain, and the default domain returns a different class.

I think that clearly every call to get should check first whether the class is already in the local classes cache. Adding such a check fixed most of the failing tests in AS trunk (only two failures left), but broke the cache tests of Kabir's implementation.

So, I have a few questions regarding this:

- 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

- 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).

- 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?

My next steps will be:

- investigate the next couple of AOP tests that are failing

- investigate the majority of scoped AOP tests that are failing

- investigate why is it that the default domain pool is returning a class that it shouldn't return at all

[kabirkhan]

[e-mail] 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());
   }

[e-mail] 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;
      }
      
     ...
   }

[e-mail] 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.

[kabirkhan]

[e-mail] wrote:

A quick update on this: for some reason, the behavior of ClassPool.get method is no longer consistent with ScopedClassPool.getLocally method.

Thinking about this, aside from the performance comments, I don't understand why this would happen? The flow should be as mentioned below

[e-mail] wrote:

The first time AOP asks for a class, during transformation, it knows that the class should be generated by the exact same classpool that corresponds to the classloader that loaded the class, so it uses getLocally.

a) So here the class should be found and added to the classpool's cache in ClassPool.classes

[e-mail] wrote:

The second time AOP asks for the class is during the same transformation (these are actually indirect calls made from inside javassist, during call weaving, for example), the class is retrieved through get. Currently, the classpool delegates the retrieval of the class to the default domain, and the default domain returns a different class.

b) So it delegates to the domain, which goes over all the classpools in the domain and calls loadLocally() on each class. This hits the cache and if not found there tries to load the class locally. When it comes to the classpool in a) it should be found in its ClassPool.classes.

The only scenario I can think of where this might not happen, is if there are two classpools in the domain loading the same class? CP1 was added to the domain first and contains Class A, and CP2 was added later and also contains Class A. If a) happens for Class A using CP2 then that will be added, but in b) when hitting the domain CP1 will be used first. Maybe we are having problems with classpools not getting unregistered as they should?

[flavia.rainone]

[e-mail] wrote:

[e-mail] 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.

[e-mail] wrote:

[e-mail] 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.

[e-mail] wrote:

[e-mail] 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.

[flavia.rainone]

[e-mail] wrote:

The only scenario I can think of where this might not happen, is if there are two classpools in the domain loading the same class? CP1 was added to the domain first and contains Class A, and CP2 was added later and also contains Class A. If a) happens for Class A using CP2 then that will be added, but in b) when hitting the domain CP1 will be used first. Maybe we are having problems with classpools not getting unregistered as they should?

I am not sure, because the tests fail even the first time I run the tests against a newly started up AS. Where else would this class be coming from, except from the new ClassPool itself?

[flavia.rainone]

After porting JBAOP-765, JBAOP-768 and JBAOP-763 to JBoss AOP branch 742, I still see a couple of failures, both related to subclassing tests.

It took me a while, but I finally discovered what is causing those failures. The problem is in the classpool structure. By some unexpected reason, the parent class pool of ExtClassLoader is a classpool representing a class loader that belongs to default domain. Since ExtClassLoader is parent of the parent of the default domain, that classpool is in the delegation chain of itself. It also belongs to the delegation chain of other classpools, which is clearly a bug.

This bug manifests itself only on corner cases, and it can be seen only by debugging Javassist. That's why it was so hard to find out. For example, one of the failures I was seeing, in AOPUnitTestCase, was happening because the access to POJO.protectedField below was not woven.

public class POJOChild extends POJO
{
 ...

 public void accessProtectedField()
   {
      protectedField += 1;
      System.out.println("protectedField = " + protectedField);
   }
}

JBoss AOP Transformer was doing everything correctly, but the javassist CodeConverter was failing to replace the accesses to protectedField above by calls to the wrapper methods of such joinpoints. I discovered that, once the CodeConverter found those accesses, it matched the name of the field correctly, but it failed to match the class that declares the protectedField, because it obtained a CtClass instance representing POJO that was not the same instance contained in the CodeConverter, even though both instances represented the same classes (one of the instances was retrieved from the correct class loader, the other one was not).

Now, I'll investigate to see why the classpool hierarchy is wrong, so I can find out how to fix it.

[flavia.rainone]

[e-mail] wrote:

Now, I'll investigate to see why the classpool hierarchy is wrong, so I can find out how to fix it.

I found out what is wrong and, since it is kind of complex, I decided to open a thread on the Javassist forum:

http://community.jboss.org/thread/147766