1 2 3 Previous Next 39 Replies Latest reply on Mar 24, 2010 7:45 PM by flavia.rainone

    ClassPool Refactoring

    flavia.rainone

      As part of my work with the ClassPools (see https://jira.jboss.org/jira/browse/JBREFLECT-3), I refactored the ClassPools at JBoss AOP. One of the purposes is to see if JBoss AOP works with the new classpools as a way of validating the new classpools:
      https://jira.jboss.org/jira/browse/JBAOP-742

      So, for this refactoring, I took the following steps:
      - extracted the code from the class pools that was related to JBoss AOP (so, code that manipulates lists for AspectManager and the like was saved)
      - removed the class pools that have been moved to the jboss classpool project (the main difference between the two groups is that, in the jboss classpool project, those classpools have no dependency on JBoss AOP stuff and, hence, they lack the code I extracted above)
      - finally, I came up with a way of integrating the extracted code in a new structure.

      The last step is the main thing about JBAOP-742. Most of the code was located at AOPClassPool. What I would have liked to have done:

      public class AOPClassPool implements ClassPool
      {
       private ClasPool delegate;
      
       // every single method of ClassPool interface will be forwarded to delegate
       public get(String name) { delegate.get(name);}
       // etc
      }
      


      The AOPClassPool above would basically forward all ClassPool calls to a delegate. This delegate could be of any ClassPool type of jboss-classpool project, thus making possible for AOP to use any class pool type provided by JBoss ClassPool. When some AOP-related stuf would have to be done as part of one of those calls, this AOPClassPool would perform the job before or after calling the delegate.

      However, ClassPool is not an interface. It is a class instead. For that reason, I tried to stick with the idea above but extending a class to make all the superclass contents useless is bad. Another problem I had to face:
      AOPClassPool aopClassPool = ....;
      CtClass clazz = aopClassPool.get("MyClass");
      Sytem.out.println("Clazz's class pool: " + clazz.getClassPool());
      


      The last line would show the delegate classpool, and not the original aopClassPool. However, I dunno that this would be important for anything other than the tests. Temporarily, I added an equals to AOPClassPool, so it can consider itself equal to its delegate (the relationship is 1 to 1, if there is a delegate, it is not being used elsewhere apart from the AOPClassPool that contains it). However, I'm not sure about this.

      I also needed to add two more things to the project:
      - JBoss AOP used the ClassPoolRepository to remove advisors when a classloader is unregistered. For that reason I created a callback at jboss-classpool: ClassPoolRepositoryCallback. The class that implements this interface at JBoss AOP is ClassLoaderRepository and basically it contains this code for removing the advisors after a classloader is unregistered.
      - the old DomainRegistry at JBoss AOP kept track of ClasLoaderDomains, Modules, and AOP Domains. When this registry was moved to classpool, I had to get rid of the AOP Domains part. For that reason, I created an AOPDomainRegistry at JBoss AOP:
      public interface AOPDomainRegistry extends DomainRegistry
      {
       boolean initMapsForLoader(ClassLoader loader, Module module, ScopedVFSClassLoaderDomain domain, ClassLoader parentUnitLoader);
      
       Domain getRegisteredDomain(ClassLoader cl);
      
       List<ScopedVFSClassLoaderDomain> getAOPDomainsForClassLoaderDomain(ClassLoaderDomain domain);
      
       ReadWriteLock getAOPDomainsLockForClassLoaderDomain(ClassLoaderDomain domain);
      }
      


      Kabir, I need you to take a look at the changes I've done so you can give me feedback on what you think :)


        • 1. Re: ClassPool Refactoring
          flavia.rainone

          Kabir has provided me feedback on this in private. He thinks that, overall, the integration is good to be ported to trunk. I'll post here the points he addressed in our private chat. These are the points that need to reviewed:

          1. The field ClassPool classPool of Instrumentor should be of type AOPClassPool.
          2. I should see if I can find a workaround for checks such as the one below:

          if (this.childFirstLookup != delegate.childFirstLookup)
           {
           delegate.childFirstLookup = this.childFirstLookup;
           }

          This check is there because childFirstLookup is a public field of ClassPool. AOPClassPool uses ClassPool as a delegate, so it can delegate to any classpool of jboss-classpool project. However, to force AOPClassPool and ClassPool to work consistently, I added these checks. This will be reviewed, but we are both afraid that maybe there is no solution for this.
          3. Should ClassPoolRepository.callback be a list instead? I've made it a reference to a single callback for performance reasons. I can change it if Kabir or Ales prefer.
          4. Maybe we should rename ClassLoaderRepository to avoid confusion with ClassPoolRepository class.
          5.In (AOP)VFSClassLoaderDomain, instead of the initMapsDone and cleanupLoaderDone methods, I could just override initMaps() and cleanupLoader() directly in AOPVFSClassLoaderDomain, call the super method and add the "Done" stuff at the end. Kabir pointed out that this may be not so simple as it looks because I need some local variables from those methods.

          I'm going to work on these points before porting the changes to trunk.

          Notice that I'll have to check if the tests are still passing and do any adaptation needed on the 742 branch... This can be done only after the classpool project is estabilized and the vfs tests are passing ok. Those tests are the subject of another thread.
          http://www.jboss.org/index.html?module=bb&op=viewtopic&p=4265325#4265325

          • 2. Re: ClassPool Refactoring
            kabirkhan

             

            "flavia.rainone@jboss.com" wrote:

            3. Should ClassPoolRepository.callback be a list instead? I've made it a reference to a single callback for performance reasons. I can change it if Kabir or Ales prefer.

            It should definitely be a list. Other systems apart from AOP might want to do some cleanup as well and so should have the option to install a callback. Performance isn't really an issue in this case, dropping classloaders will normally happen on undeploy which involves a lot more work than creating an extra iterator :-)

            • 3. Re: ClassPool Refactoring
              flavia.rainone

              After the jboss-classpool 1.0.0 alpha1 has been released, I ran the AOP testsuite on AS trunk with JBAOP-742 branch.

              What I discovered was a tremendous overhead. The tests, that used to take about 5 minutes to run, were now taking over 21 minutes .

              In particular, the AOPTestUnitCase used to take about 11 seconds, and now it was taking 150seconds to run.

              To improve that performance:

              - I got rid of the AOPClassPool, whose existence seemed necessary when I started doing the refactoring, but it is not. Amazingly, the overhead caused by an extra level of delegation in the classpool chain was such that AOPTestUnitTestCase runtime dropped to 30 seconds.

              - I got rid of an extra delegation to parent class pool that was being done in the ClassPoolDomain (see https://jira.jboss.org/jira/browse/JBREFLECT-89).

              - I got rid of the "wrapped.setStackTrace(e.getStackTrace())" line in the get method of BaseClassPool:

               

                 @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
                    {
                       CtClass clazz = super.get(classname);
                       if (trace)
                       {
                          logger.trace(this + " completed get of " + classname + " " + getClassPoolLogStringForClass(clazz));
                       }
                       return clazz;
                    }
                    catch (NotFoundException e)
                    {
                       // AutoGenerated
                       NotFoundException wrapped = new NotFoundException(e.getMessage() + " could not be found from " + this);
               HERE>>>>>        wrapped.setStackTrace(e.getStackTrace());
                       throw wrapped;
                    }
                 }
              

               

              The last two improvements led AOPUnitTestCase to take 15 seconds.

               

              I didn't commit the last one because I would like to first know if there is a very good reason for keeping that catch block. Otherwise, I'll remove it as the e.getStackTrace() was causing overhead.

               

              It is important to notice that the previous two bottlenecks were already there before the refactoring. So, what has changed with the classpools that evidenced those bottlenecks? The answer is that now we have DefaultClassPool as parent of app class loader, so that we don't end up with duplicates of java.lang.String, java.util.Collection and other bootstrap classes, which is a good thing. However, that is responsible for a jump in the number of invocations to ClassPool.get, from 135000 to about 330000 during AOPTestUnitCase execution.  (notice that we are talking about the total number of calls to ClassPool.get, including delegation ones). With JBREFLECT-89, the total number of calls dropped 228000 during the test execution.

               

              Finally, I detected a great deal of calls to ClassPool.get("org.jboss.aop.microcontainer.annotations.DisableAOP"). The calls are being made by org.jboss.annotation.factory.javassist.DefaultValueAnnotationValidator.getDeclaredMethods(Class<?>), and I'm confident that this can be improved somehow (such as caching DisableAOP ctClass). I think improving this will have some impact on the startup time of AS.

              I'll be away for the next week, so, if anybody wants to try it, feel free and let me know the results .

              • 4. Re: ClassPool Refactoring
                kabirkhan

                flavia.rainone@jboss.com wrote:

                 

                I didn't commit the last one because I would like to first know if there is a very good reason for keeping that catch block. Otherwise, I'll remove it as the e.getStackTrace() was causing overhead

                It is just to have extra information about which classpool could not find it, but you could do that with trace logging instead:

                 

                catch(NotFoundException e)
                {
                   if (trace)
                      log.trace(classname + " could not be found from " + this, e);
                   throw e;
                }
                
                • 5. Re: ClassPool Refactoring
                  alesj

                  Finally, I detected a great deal of calls to ClassPool.get("org.jboss.aop.microcontainer.annotations.DisableAOP"). The calls are being made by org.jboss.annotation.factory.javassist.DefaultValueAnnotationValidator.getDeclaredMethods(Class<?>), and I'm confident that this can be improved somehow (such as caching DisableAOP ctClass). I think improving this will have some impact on the startup time of AS.

                  We're deprecating this annotation anyway.

                  But we're replacing it with just the opposite - EnableAOP, so not a lot different from performance perspective.

                  Kabir, any idea on how to optimize (e.g. cache) this?

                  • 6. Re: ClassPool Refactoring
                    alesj

                    It is just to have extra information about which classpool could not find it, but you could do that with trace logging instead:

                     

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

                    Done.

                    • 7. Re: ClassPool Refactoring
                      kabirkhan

                      alesj wrote:

                       

                      Finally, I detected a great deal of calls to ClassPool.get("org.jboss.aop.microcontainer.annotations.DisableAOP"). The calls are being made by org.jboss.annotation.factory.javassist.DefaultValueAnnotationValidator.getDeclaredMethods(Class<?>), and I'm confident that this can be improved somehow (such as caching DisableAOP ctClass). I think improving this will have some impact on the startup time of AS.

                      We're deprecating this annotation anyway.

                      But we're replacing it with just the opposite - EnableAOP, so not a lot different from performance perspective.

                      Kabir, any idea on how to optimize (e.g. cache) this?

                       

                      The quick and dirty way would be at DefaulValueAnnotationValidator level

                      -a WeakHashMap<Class<?>, CtClass>, or

                      -be able to configure it with annotations it should cache

                       

                      But I guess the real and harder solution is that caching at class pool level could be improved? What seems to happen now is that when trying to load a CtClass we end up in Base-/JBossClClassPoolDomain which checks each relavant pool and parent domain for a locally cached CtClass which is simple but probably more work than necessary. Maybe these lookups should be cached in the initiating pool, with some invalidation if the domain (or its parent domain) has pools added/removed?

                      • 8. Re: ClassPool Refactoring
                        kabirkhan

                        kabir.khan@jboss.com wrote:

                        Maybe these lookups should be cached in the initiating pool, with some invalidation if the domain (or its parent domain) has pools added/removed?

                        Or caching at the domain level might make more sense

                        • 9. Re: ClassPool Refactoring
                          alesj

                          Or caching at the domain level might make more sense

                          We should definitely do some caching,

                          otherwise we'll end up like MC's CL who suffered heavily by not caching at the begining.

                           

                          Can you have a look at this?

                          And perhaps we can still push it into M2?

                          • 10. Re: ClassPool Refactoring
                            kabirkhan

                            alesj wrote:

                             


                            Or caching at the domain level might make more sense

                            We should definitely do some caching,

                            otherwise we'll end up like MC's CL who suffered heavily by not caching at the begining.

                             

                            Can you have a look at this?

                            And perhaps we can still push it into M2?

                            OK

                            • 11. Re: ClassPool Refactoring
                              kabirkhan
                              I also see that we are using synchronized quite a lot in heavily hit places like BaseClassPoolDomain.getCachedOrCreate(), I'll try to refactor to use R/W locks
                              • 12. Re: ClassPool Refactoring
                                kabirkhan

                                I have refactored and committed the changes against https://jira.jboss.org/jira/browse/JBREFLECT-90 (I think JBREFLECT is the project the classpool stuff belongs under?).

                                 

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

                                • 13. Re: ClassPool Refactoring
                                  kabirkhan

                                  kabir.khan@jboss.com wrote:

                                   

                                  I have refactored and committed the changes

                                  By this I mean the changes for improving concurrency

                                  • 14. Re: ClassPool Refactoring
                                    kabirkhan
                                    The API change has been commited against https://jira.jboss.org/jira/browse/JBREFLECT-91
                                    1 2 3 Previous Next