JBossDeveloper

All tests are passing now in my system, which allowed me to validate two previous fixes Kabir and I implemented for the problem of duplicate CtClasses.

Kabir's fix consists of JBAOP-766 :

public class DelegatingClassPool extends BaseClassPool
{
...
 public CtClass getCached(String classname)
   {
>>>      //TODO Not 100 sure this is correct, but it seems to do the job where repeated calls to get() on a pool in a hierarchy returns a different instance of the class
>>>      CtClass clazz = super.getCachedLocally(classname);
>>>      if (clazz != null)
>>>         return clazz;  
      
      if (isGeneratedClass(classname))
      {
         return null;
      }
      return domain.getCachedOrCreate(this, classname, false);
   }
}

My fix has never been comitted and consists of adding a check on the classes cache to BaseClassPool.get:

public class BaseClassPool extends AbstractClassPool
{
 ...
 @Override
   public final CtClass get(String classname) throws NotFoundException 
   {
      boolean trace = logger.isTraceEnabled();
      if (trace) logger.trace(this + " initiating get of " + classname);

      if (this.getClassLoader() == null)
      {
         throw new IllegalStateException("Illegal call. " + " A class cannot be retrieved from ClassPool " +
                                         this + " because the corresponding ClassLoader is garbage collected");
      }
      try
      {
>>>         if (this.classes.containsKey(classname))
>>>         {
>>>            return (CtClass) classes.get(classname); 
>>>         }
         CtClass clazz = super.get(classname);
         if (trace)
            logger.trace(this + " completed get of " + classname + " " + getClassPoolLogStringForClass(clazz));

         return clazz;
      }
      catch (NotFoundException e)
      {
         if (trace)
            logger.trace(classname + " could not be found from " + this, e);
         throw e;
      }
   }
}

It turns out that, once all tests were fixed for issue JBREFLECT-94, those fixes weren't needed anymore, which means that JBREFLECT-94 was the real issue.

Now I'm left with the dillema of whether should I commit those fixes. I have started running some tests to try to figure out if any of those bring some improvement, and it seems that looking up classes cache in BaseClassLoader.get does bring a few improvements to the performance of aop tests, but I need to run those tests more times in order to do a reasonable investigation.

While working with Kabir to fix the AOP tests for AS, he pointed out that there could be an issue related to my fix. It seemed obviously right to me to look in the classes cache (a cache declared in ScopedClassPool that contains all classes created by the current class pool) before doing anything else. To me, if we have a hit in the cache, it means that the current pool is the one that is supposed to load the class, and that there is no need to look further by calling super.get method. But Kabir raised the following possibility. What if the class pool scenario (a reflection of the class loader/domain scenario) changes after the current class pool loaded the class, in a way that this class pool is no longer the one responsible for loading that class? In that case, looking for the class in the classes cache would be a serious mistake. Is this scenario possible?

Regarding Kabir's fix, I'm also looking for any opnion regarding whether it should be committed. It seems, though, that adding both fixes brings some small performance penalty. I'll continue running tests to be able to assert whether this is really true, and what is the penalty we're talking about.

flavia.rainone@jboss.com wrote:

 

All tests are passing now in my system, which allowed me to validate two previous fixes Kabir and I implemented for the problem of duplicate CtClasses.

Kabir's fix consists of JBAOP-766 :

 

public class DelegatingClassPool extends BaseClassPool
{
...
 public CtClass getCached(String classname)
   {
>>>      //TODO Not 100 sure this is correct, but it seems to do the job where repeated calls to get() on a pool in a hierarchy returns a different instance of the class
>>>      CtClass clazz = super.getCachedLocally(classname);
>>>      if (clazz != null)
>>>         return clazz;  
      
      if (isGeneratedClass(classname))
      {
         return null;
      }
      return domain.getCachedOrCreate(this, classname, false);
   }
}

 

 

My fix has never been comitted and consists of adding a check on the classes cache to BaseClassPool.get:

 

public class BaseClassPool extends AbstractClassPool
{
 ...
 @Override
   public final CtClass get(String classname) throws NotFoundException 
   {
      boolean trace = logger.isTraceEnabled();
      if (trace) logger.trace(this + " initiating get of " + classname);
 
      if (this.getClassLoader() == null)
      {
         throw new IllegalStateException("Illegal call. " + " A class cannot be retrieved from ClassPool " +
                                         this + " because the corresponding ClassLoader is garbage collected");
      }
      try
      {
>>>         if (this.classes.containsKey(classname))
>>>         {
>>>            return (CtClass) classes.get(classname); 
>>>         }
         CtClass clazz = super.get(classname);
         if (trace)
            logger.trace(this + " completed get of " + classname + " " + getClassPoolLogStringForClass(clazz));
 
         return clazz;
      }
      catch (NotFoundException e)
      {
         if (trace)
            logger.trace(classname + " could not be found from " + this, e);
         throw e;
      }
   }
}

 

It turns out that, once all tests were fixed for issue JBREFLECT-94, those fixes weren't needed anymore, which means that JBREFLECT-94 was the real issue.

 

Now I'm left with the dillema of whether should I commit those fixes. I have started running some tests to try to figure out if any of those bring some improvement, and it seems that looking up classes cache in BaseClassLoader.get does bring a few improvements to the performance of aop tests, but I need to run those tests more times in order to do a reasonable investigation.

 

While working with Kabir to fix the AOP tests for AS, he pointed out that there could be an issue related to my fix. It seemed obviously right to me to look in the classes cache (a cache declared in ScopedClassPool that contains all classes created by the current class pool) before doing anything else. To me, if we have a hit in the cache, it means that the current pool is the one that is supposed to load the class, and that there is no need to look further by calling super.get method. But Kabir raised the following possibility. What if the class pool scenario (a reflection of the class loader/domain scenario) changes after the current class pool loaded the class, in a way that this class pool is no longer the one responsible for loading that class? In that case, looking for the class in the classes cache would be a serious mistake. Is this scenario possible?

Regarding Kabir's fix, I'm also looking for any opnion regarding whether it should be committed. It seems, though, that adding both fixes brings some small performance penalty. I'll continue running tests to be able to assert whether this is really true, and what is the penalty we're talking about.

I ran the performance tests, and concluded that there is not performance penalty in including both "fixes". On the contrary, I have seen some small improvements with those  (AOPUnitTestCase runs 2.5 seconds faster with the fix on BaseClassPool; as starts 200 ms faster in all mode with both fixes).

I'm comitting this.

flavia.rainone@jboss.com wrote:

 

My fix has never been comitted and consists of adding a check on the classes cache to BaseClassPool.get:

 

public class BaseClassPool extends AbstractClassPool
{
 ...
 @Override
   public final CtClass get(String classname) throws NotFoundException 
   {
      boolean trace = logger.isTraceEnabled();
      if (trace) logger.trace(this + " initiating get of " + classname);
 
      if (this.getClassLoader() == null)
      {
         throw new IllegalStateException("Illegal call. " + " A class cannot be retrieved from ClassPool " +
                                         this + " because the corresponding ClassLoader is garbage collected");
      }
      try
      {
>>>         if (this.classes.containsKey(classname))
>>>         {
>>>            return (CtClass) classes.get(classname); 
>>>         }
         CtClass clazz = super.get(classname);
         if (trace)
            logger.trace(this + " completed get of " + classname + " " + getClassPoolLogStringForClass(clazz));
 
         return clazz;
      }
      catch (NotFoundException e)
      {
         if (trace)
            logger.trace(classname + " could not be found from " + this, e);
         throw e;
      }
   }
}

[....]

 

While working with Kabir to fix the AOP tests for AS, he pointed out that there could be an issue related to my fix. It seemed obviously right to me to look in the classes cache (a cache declared in ScopedClassPool that contains all classes created by the current class pool) before doing anything else. To me, if we have a hit in the cache, it means that the current pool is the one that is supposed to load the class, and that there is no need to look further by calling super.get method. But Kabir raised the following possibility. What if the class pool scenario (a reflection of the class loader/domain scenario) changes after the current class pool loaded the class, in a way that this class pool is no longer the one responsible for loading that class? In that case, looking for the class in the classes cache would be a serious mistake. Is this scenario possible?

Does anybody know the answer to this question? I can't commit it without knowing the answer.

alesj wrote:

 

It depends how and were things are cached.

This cache I'm talking about is one that is ScopedClassPool. It contains only the ctClasses created by that particular CP.

alesj wrote:

 

Since, of course you should be able to re-deploy things, while Classpool still reflect proper/actual state.

Undeploy should evict related things from cache as well.

 

Re-deploy is the only scenario I see as valid, that would change the structure/state.

If something is undeployed, isn't the ClassLoader simply "thrown away"? The same will happen to the CP.

Plus, if something is redeployed, isn't a new ClassLoader created for the new deployment? Since every CP is associated with a CL, we would hence have a new CP for that new CL, and the old cache would be automatically discarded.

kabir.khan@jboss.com wrote:

 

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

I've tested AS + CP + JBoss AOP 742 against AS trunk with the cache enabled and we got 19.3% of improvement on AOPUnitTestCase performance and 6.8% of performance improvement on the entire AOP test suite. We got no improvement on AS startup time, however. Still, the results show what we expected. At the points that use CP more extensively, we see a considerable gain in performance with the cache enabled.

I created a Jira for integrating all work that has been done as part of JBAOP-742 to JBoss AOP trunk:

https://jira.jboss.org/jira/browse/JBAOP-772

Notice that the task also includes creating a new 2.2 Branch. Since trunk contains a few issues that have not been throughly tested yet, this branch will contain the same class pool refactoring work but will be based on Branch 2_1 instead of trunk.

Flavia Rainone wrote:

 

I created a Jira for integrating all work that has been done as part of JBAOP-742 to JBoss AOP trunk:

https://jira.jboss.org/jira/browse/JBAOP-772

This is done.