14 Replies Latest reply on May 28, 2009 8:29 AM by jaikiran pai

    Various performance issues in core projects

    jaikiran pai Master

      While working on EJBTHREE-1800, we found that most of the issues were either related to AOP or MC. So i created a simple plain MC app (no EJBs in picture) with 101 MC beans and each MC bean having 100 methods each. Like this:

      package org.myapp.mc;
      
      import org.jboss.beans.metadata.api.annotations.*;
      
      public class MCBean2
      {
      
       @Inject
       private SomeOtherInterface otherBean;
      
      
       public SomeOtherInterface getOtherBean()
       {
       return this.otherBean;
       }
      
      
      
       public void method1(){}
      public void method2(){}
      public void method3(){}
      public void method4(){}
      public void method5(){}
      public void method6(){}
      public void method7(){}
      public void method8(){}
      public void method9(){}
      


      Each MC bean has just one plain @Inject and 100 non-annotated methods. When i deployed this on my system, it took close to 2 seconds for this to be deployed. There's nothing fancy in this application, so i would have expected this to be faster. But this timing aligns with what we are seeing with the EJB3 deployment (we do not deploy the EJB impl class as MC beans but we do deploy the EJB containers as MC beans).

      Based on what i could see for this plain MC application in JProfiler, here are the issues (so far):

      1) As mentioned in the other thread, MC looks for annotations on every method on every MC bean for annotation plugins (org.jboss.kernel.plugins.annotations.CommonAnnotationAdapter). So 100 methods * 101 beans. During this process, the call leads to org.jboss.metadata.plugins.loader.reflection.AnnotatedElementMetaDataLoader.getComponentMetaDataRetrieval which has this code:

      SecurityActions.findMethod(clazz, signature.getName(), signature.getParametersTypes(clazz));
      

      similarly for constructors and fields:
      constructor = SecurityActions.findConstructor(clazz, signature.getParametersTypes(clazz));
      
      field = SecurityActions.findField(clazz, signature.getName());
      

      This call ultimately leads to org.jboss.reflect.plugins.introspection.ReflectionUtils.findMethod/findField/findConstructor, which have code like this:

      public static Method findMethod(Class<?> clazz, String name, Class<?>... parameterTypes)
       {
       if (clazz == null)
       return null;
      
       Method[] methods = clazz.getDeclaredMethods();
       if (methods.length != 0)
       {
       for (Method method : methods)
       {
       if (method.getName().equals(name))
       {
       Class<?>[] methodParams = method.getParameterTypes();
       if (methodParams.length != 0)
       {
       if (Arrays.equals(methodParams, parameterTypes))
       return method;
       }
       else if (parameterTypes == null || parameterTypes.length == 0)
       return method;
       }
       }
       }
       return findMethod(clazz.getSuperclass(), name, parameterTypes);
       }
      
      
      


      Same applies for the findConstructor and findField. As can be seen, this method calls the expensive Class.getDeclaredMethods() and then iterates over the method to figure out the correct method. Remember, this method gets called for each method on each MC bean (100 * 101 for this application alone) which ultimately leads to performance issues. Profiler shows me there are 33232 calls to the Class.getDeclaredMethods() each taking an average of 297 micro sec. I changed the implementation of these methods (findMethod, findConstructor and findField) and see good enough performance improvements:

      public static Method findMethod(Class<?> clazz, String name, Class<?>... parameterTypes)
       {
       if (clazz == null)
       return null;
      
       try
       {
       Method method = clazz.getDeclaredMethod(name, parameterTypes);
       return method;
       }
       catch (SecurityException se)
       {
       throw se;
       }
       catch (NoSuchMethodException nsme)
       {
       // check on superclass if present
       if (clazz.getSuperclass()!=null)
       {
       return findMethod(clazz.getSuperclass(), name, parameterTypes);
       }
       else
       {
       return null;
       }
      
       }
      
       }
      



      The calls to the less expensive Class.getDeclaredMethod and Class.getDeclaredField (and so on..) show that the average time of the calls has drastically dropped down to 9 micro sec.

      This issue is similar to the one i reported here (in a different class) http://www.jboss.org/index.html?module=bb&op=viewtopic&t=154875


      2) The org.jboss.kernel.plugins.annotations.CommonAnnotationAdapter iterates over each method to apply the annotation plugins for those methods:

      ...//code trimmed
      Set<MethodInfo> methods = info.getMethods();
       if (methods != null && methods.isEmpty() == false)
       {
       for(MethodInfo mi : methods)
       {
       ClassInfo declaringCI = mi.getDeclaringClass();
       // direct == check is OK
       if (declaringCI != objectTI && visitedMethods.contains(mi) == false)
       {
       Signature mis = new MethodSignature(mi);
       MetaData cmdr = retrieval.getComponentMetaData(mis);
       if (cmdr != null)
       {
       methodPlugins = getPlugins(ElementType.METHOD, METHOD_FILTER, annotationClasses);
       for(T plugin : methodPlugins)
       {
       if (isApplyPhase)
       applyPlugin(plugin, mi, cmdr, handle);
       else
       cleanPlugin(plugin, mi, cmdr, handle);
       }
      //code trimmed
      



      As can be seen, during this process the getPlugins(...) is called for each method even though the getPlugins(...) is not per method specific. The getPlugins internally iterates over the available plugins (which are a lot) and then applies filtering for each annotation class and does other stuff. The call to this method can be moved out of the loop to improve the performance:

       Set<MethodInfo> methods = info.getMethods();
       if (methods != null && methods.isEmpty() == false)
       {
       //moved out of loop
       methodPlugins = getPlugins(ElementType.METHOD, METHOD_FILTER, annotationClasses);
       for(MethodInfo mi : methods)
       {
       ClassInfo declaringCI = mi.getDeclaringClass();
       // direct == check is OK
       if (declaringCI != objectTI && visitedMethods.contains(mi) == false)
       {
       Signature mis = new MethodSignature(mi);
       MetaData cmdr = retrieval.getComponentMetaData(mis);
      
      


      This too shows an improvement in the performance.

      3) The org.jboss.kernel.plugins.annotations.CommonAnnotationAdapter also has a piece of code which looks for static methods:

      // static methods
       MethodInfo[] staticMethods = getStaticMethods(classInfo);
       if (staticMethods != null && staticMethods.length != 0)
       {
       for(MethodInfo smi : staticMethods)
       {
      
       if (smi.isStatic() && smi.isPublic())
       {
       Signature mis = new MethodSignature(smi);
       MetaData cmdr = retrieval.getComponentMetaData(mis);
       if (cmdr != null)
       {
       if (methodPlugins == null)
       methodPlugins = getPlugins(ElementType.METHOD, METHOD_FILTER, annotationClasses);
      
       for(T plugin : methodPlugins)
       {
       if (isApplyPhase)
       applyPlugin(plugin, smi, cmdr, handle);
       else
       cleanPlugin(plugin, smi, cmdr, handle);
       }
       }
      
       /**
       * Get the static methods of class info.
       *
       * @param classInfo the class info
       * @return the static methods
       */
       protected MethodInfo[] getStaticMethods(ClassInfo classInfo)
       {
       return classInfo.getDeclaredMethods();
       }
      


      I think the call to classInfo.getDeclaredMethods() can again be avoided, since we already have the methods of this class (a few lines before this call). So this change/fix:

       // static methods
       if (methodPlugins == null)
       methodPlugins = getPlugins(ElementType.METHOD, METHOD_FILTER, annotationClasses);
      
       for (MethodInfo method : methods)
       {
       // if the method was already visited, then no point processing it again
       if (!visitedMethods.contains(method))
       {
       // ensure that the method is static and is declared in the class being processed
       if(method.isStatic() && method.getDeclaringClass().equals(classInfo))
       {
       Signature mis = new MethodSignature(method);
       MetaData cmdr = retrieval.getComponentMetaData(mis);
      
       if (cmdr != null)
       {
       for(T plugin : methodPlugins)
       {
       if (isApplyPhase)
       applyPlugin(plugin, method, cmdr, handle);
       else
       cleanPlugin(plugin, method, cmdr, handle);
       }
       }
       else if (trace)
       log.trace("No annotations for " + method);
       }
       }
      
       }
      


      4) Deployers - I see that every MC bean that gets deployed, also gets registered as a MBean. Is this required? During the MBean registration, i saw this piece of code (which Jprofiler shows as expensive, given the number of MC beans being registered as MBean) in org/jboss/deployers/structure/spi/helpers/ComponentDeploymentContext.java:

      public ObjectName getObjectName()
       {
       if (objectName == null)
       {
       String name = getName();
       name = name.replace("\"", """);
       String temp = "jboss.deployment:id=\"" + name + "\",type=Component";
      


      The String.replace is marked as a costly operation. I changed this to:
      Index: src/main/java/org/jboss/deployers/structure/spi/helpers/ComponentDeploymentContext.java
      ===================================================================
      --- src/main/java/org/jboss/deployers/structure/spi/helpers/ComponentDeploymentContext.java (revision 87763)
      +++ src/main/java/org/jboss/deployers/structure/spi/helpers/ComponentDeploymentContext.java (working copy)
      @@ -139,8 +139,9 @@
       if (objectName == null)
       {
       String name = getName();
      - name = name.replace("\"", """);
      - String temp = "jboss.deployment:id=\"" + name + "\",type=Component";
      + //name = name.replace("\"", """);
      + name = ObjectName.quote(name);
      + String temp = "jboss.deployment:id=" + name + ",type=Component";
       try
       {
       objectName = new ObjectName(temp);
      
      


      The question however still remains, do we really need to register each MC bean as a MBean?

      With all these changes (across various projects), i have been able to deploy that sample application now within around 1.1 seconds(which i think is still on the higher side). I am going to continue looking into other issues and see how much improvement it brings in to the EJB3 deployments.

      P.S: I have all the patches ready if anyone wants to take a look.



        • 2. Re: Various performance issues in core projects
          jaikiran pai Master

           

          "jaikiran" wrote:
          Profiler shows me there are 33232 calls to the Class.getDeclaredMethods() each taking an average of 297 micro sec. I changed the implementation of these methods (findMethod, findConstructor and findField) and see good enough performance improvements


          Carlo and me have been playing with various combinations of class hierarchy and number of methods. The existing implementation in ReflectionUtils (i.e. call to getDeclaredMethods()) performs better when the classes are hierarchical and the numbers of methods in this hierarchy is less. However, if the number of methods starts increasing or if there is no/less hierarchy involved in the classes, then the proposed change performs better.
          So its really a question of which is the right scenario.

          As for the rest of the changes/questions mentioned in my previous post, they still hold good.


          • 3. Re: Various performance issues in core projects
            jaikiran pai Master

             

            "jaikiran" wrote:


            The question however still remains, do we really need to register each MC bean as a MBean?



            Just looked at a sample MBean corresponding to a MC Bean, that gets registered by default (ex: jboss.deployment:id="HDScanner",type=Component). This MBean and other similar MBeans that are registered for each MC bean provide debug level information. The MBean has a listAttachments() method which doesn't provide much except the object ids/classnames of the attachments. Even the attributes of these MBeans are the kind of information that's just useful (IMO) for low level debugging. So shouldn't registering these as MBeans be an optional thing, and by default be turned off?


            • 4. Re: Various performance issues in core projects
              Ales Justin Master

              1) Method(Info)::getDeclaredMethod usage
              * Config::locateMethodInfo
              ** Static methods check

              I'll check why we used the (*), but afair there was a legit reason.
              And I'll see if I can handle static methods in a single iteration as well.

              2) Method plugins out of the loop

              Yeah, this is a bug, and I've already fixed it.

              3) ObjectName quote

              I see a different code in CDC:

               name = name.replace("\"", """);
               String temp = "jboss.deployment:id=\"" + name + "\",type=Component";
              

              where I don't see a " in your code.

              4) Deployment as an mbean

              This flag already exists, see DeployersImpl::isRegisterMBeans.

              • 5. Re: Various performance issues in core projects
                Ales Justin Master

                 

                "alesj" wrote:

                3) ObjectName quote

                I see a different code in CDC:
                 name = name.replace("\"", """);
                 String temp = "jboss.deployment:id=\"" + name + "\",type=Component";
                

                where I don't see a " in your code.

                Ah, the forum "eats" the ". :-)

                But the ObjectName::quote doesn't handle this as we would like:
                (probably a readable web text)
                 public static String quote(String s)
                 throws NullPointerException {
                 final StringBuffer buf = new StringBuffer("\"");
                 final int len = s.length();
                 for (int i = 0; i < len; i++) {
                 char c = s.charAt(i);
                 switch (c) {
                 case '\n':
                 c = 'n';
                 // fall in...
                 case '\\':
                 case '\"':
                 case '*':
                 case '?':
                 buf.append('\\');
                 break;
                 }
                 buf.append(c);
                 }
                 buf.append('"');
                 return buf.toString();
                 }
                



                • 6. Re: Various performance issues in core projects
                  Ales Justin Master

                   

                  "alesj" wrote:

                  And I'll see if I can handle static methods in a single iteration as well.

                   Set<MethodInfo> methods = info.getMethods();
                  

                  This doesn't return static methods,
                  hence we need to call ClassInfo::getDeclaredMethods which does.



                  • 7. Re: Various performance issues in core projects
                    jaikiran pai Master

                     

                    "alesj" wrote:

                    4) Deployment as an mbean

                    This flag already exists, see DeployersImpl::isRegisterMBeans.


                    But i don't see this flag getting queried from ComponentDeploymentContext where the registering of MBeans is happening:

                    public void addComponent(DeploymentContext component)
                     {
                     if (component == null)
                     throw new IllegalArgumentException("Null component");
                     components.add(component);
                     if (server != null)
                     registerMBeans(component, true, true);
                     }
                    


                    Furthermore, any reason why this flag is at DeployersImpl level instead of per MC bean/context level? Allowing this flag to be at per bean/context, and by default having it set to false, would help in better controlling the individual deployments without affecting the rest of the deployments in the server.


                    • 8. Re: Various performance issues in core projects
                      jaikiran pai Master

                       

                      "alesj" wrote:

                      2) Method plugins out of the loop

                      Yeah, this is a bug, and I've already fixed it.

                      Ales, will this be available in the to-be-released JBossAS-5.1.0.GA version?



                      • 9. Re: Various performance issues in core projects
                        Adrian Brock Master

                        3) Using ObjectName.quote() is probably not the same thing. It would lead to a mangled
                        and probably unreadable ObjectName.

                        4) The correct way to not register (sub-)deployment/components in JMX is
                        to remove the @JMX annotation from the DeployersImpl bean in conf/bootstrap/deployers.xml

                        If the DeployersImpl is not registered in JMX then neither will be the deployments.

                        This has nothing directly to do with the bean.

                        It is letting you look (using JMX) at what deployers the deployment/component went
                        through, the attachments (deployment descriptors) associated with the
                        deployment/component and maybe how long the processing took for each deployer
                        if you enable the stats.

                        It's just debug information so it is optional.

                        • 10. Re: Various performance issues in core projects
                          jaikiran pai Master

                           

                          "adrian@jboss.org" wrote:
                          3) Using ObjectName.quote() is probably not the same thing. It would lead to a mangled
                          and probably unreadable ObjectName.

                          Thanks, i wasn't aware of that. I did notice Ales mentioning similar thing about this, but did not get a chance to try it out. Letting it remain the way it is won't be a big deal, once we take care of not registering the optional MBeans, by default.



                          • 11. Re: Various performance issues in core projects
                            jaikiran pai Master

                             

                            "adrian@jboss.org" wrote:

                            4) The correct way to not register (sub-)deployment/components in JMX is
                            to remove the @JMX annotation from the DeployersImpl bean in conf/bootstrap/deployers.xml

                            If the DeployersImpl is not registered in JMX then neither will be the deployments.

                            This has nothing directly to do with the bean.

                            It is letting you look (using JMX) at what deployers the deployment/component went
                            through, the attachments (deployment descriptors) associated with the
                            deployment/component and maybe how long the processing took for each deployer
                            if you enable the stats.

                            It's just debug information so it is optional.


                            If no one has any objections, i am going to comment out the JMX annotation on Deployers MC bean in conf/bootstrap/deployers.xml of AS trunk. I'll create a JIRA


                            • 13. Re: Various performance issues in core projects
                              Ales Justin Master

                               

                              "jaikiran" wrote:

                              If no one has any objections, i am going to comment out the JMX annotation on Deployers MC bean in conf/bootstrap/deployers.xml of AS trunk. I'll create a JIRA

                              I guess we need a bigger audience to confirm this. ;-)
                              e.g. jboss-dev to check if any user/dev complains
                              (as not everybody follows this forum, where ml is more observed)