8 Replies Latest reply on Feb 18, 2010 11:00 AM by kabirkhan

    AnnotatedElementMetaDataLoader component metadata optimization

    kabirkhan

      Inspired by http://community.jboss.org/thread/96937?tstart=0 I did some standalone benchmarks of beans with a lot of methods, and found a bottleneck in CommonAnnotationAdapter which needs to obtain the component metadata for every single constructor/method/property.

       

      The problem is that as far as I can tell:

      • MemoryMetaDataLoader delegates to AbstractMutableComponentMetaDataLoader.getComponentMetaDataRetrieval(), which returns null if there is no component metadata:
      •    public MetaDataRetrieval getComponentMetaDataRetrieval(Signature signature)
           {
              if (components == null)
                 return null;
        
              return components.get(signature);
           }
        
      • AnnotatedElementMetaDataLoader on the other hand will always return a loader as long as the passed in signature can be found
      •    public MetaDataRetrieval getComponentMetaDataRetrieval(Signature signature)
           {
              if (signature == null)
                 return null;
        
              if (annotated instanceof Class)
              {
                 Class<?> clazz = Class.class.cast(annotated);
                 if (signature instanceof ConstructorSignature)
                 {
                    ConstructorSignature constructorSignature = (ConstructorSignature) signature;
                    Constructor<?> constructor = constructorSignature.getConstructor();
                    if (constructor == null)
                       constructor = SecurityActions.findConstructor(clazz, signature.getParametersTypes(clazz));
                    if (constructor == null)
                    {
                       if (log.isTraceEnabled())
                          log.trace("Constructor with signature " + signature + " does not exist on class " + clazz.getName());
                       return null;
                    }
                    return new AnnotatedElementMetaDataLoader(constructor);
                 }
                 //Same for fields, methods, parameters

       

      The problem with what AnnotatedElementMetaDataLoader does is that even if the constructor in question has no annotations, it still needs to instantiate the "empty" AnnotatedElementMetaDataLoader, which is quite costly. The bulk of that cost goes into creating the ScopeKey, and instantiating ScopeKey's ConcurrentSkipListMap and adding to it take up most of that time.

       

      In an attempt to cut down on this cost I tried the following fix which cuts ~650ms off the AS boot time:

         public MetaDataRetrieval getComponentMetaDataRetrieval(Signature signature)
         {
            if (signature == null)
               return null;
      
            if (annotated instanceof Class)
            {
               Class<?> clazz = Class.class.cast(annotated);
               if (signature instanceof ConstructorSignature)
               {
                  ConstructorSignature constructorSignature = (ConstructorSignature) signature;
                  Constructor<?> constructor = constructorSignature.getConstructor();
                  if (constructor == null)
                     constructor = SecurityActions.findConstructor(clazz, signature.getParametersTypes(clazz));
                  if (constructor == null)
                  {
                     if (log.isTraceEnabled())
                        log.trace("Constructor with signature " + signature + " does not exist on class " + clazz.getName());
                     return null;
                  }
      
                  //Return null to avoid overhead of initializing AnnotatedElementMetaDataLoader's ScopeKey
                  //which shows up to be a big bottleneck in AS startup time          
                  if (constructor.getAnnotations().length == 0)
                     return NullAnnotatedElementMetaDataLoader.INSTANCE;
                  return new AnnotatedElementMetaDataLoader(constructor);
               }
               //Same for fields, methods, parameters

       

      The problem with this is that it makes some tests fail in MDR, since they assume that there will always be a ComponentMetaData. Those tests are:

       

      MetaDataAllTestSuite

      MetaData Tests

      MetaDataLoader Tests

      Reflection MetaDataLoader Tests

      org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderComponentBasicAnnotationsUnitTestCase

      testFieldEmpty(org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderComponentBasicAnnotationsUnitTestCase)

      junit.framework.AssertionFailedError: null

      at junit.framework.Assert.fail(Assert.java:47)

      at junit.framework.Assert.assertTrue(Assert.java:20)

      at junit.framework.Assert.assertNotNull(Assert.java:214)

      at junit.framework.Assert.assertNotNull(Assert.java:207)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testEmpty(ComponentBasicAnnotationsTest.java:102)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testFieldEmpty(ComponentBasicAnnotationsTest.java:69)

       

      testConstructorEmpty(org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderComponentBasicAnnotationsUnitTestCase)

      junit.framework.AssertionFailedError: null

      at junit.framework.Assert.fail(Assert.java:47)

      at junit.framework.Assert.assertTrue(Assert.java:20)

      at junit.framework.Assert.assertNotNull(Assert.java:214)

      at junit.framework.Assert.assertNotNull(Assert.java:207)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testEmpty(ComponentBasicAnnotationsTest.java:102)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testConstructorEmpty(ComponentBasicAnnotationsTest.java:76)

       

      testMethodEmpty(org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderComponentBasicAnnotationsUnitTestCase)

      junit.framework.AssertionFailedError: null

      at junit.framework.Assert.fail(Assert.java:47)

      at junit.framework.Assert.assertTrue(Assert.java:20)

      at junit.framework.Assert.assertNotNull(Assert.java:214)

      at junit.framework.Assert.assertNotNull(Assert.java:207)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testEmpty(ComponentBasicAnnotationsTest.java:102)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testMethodEmpty(ComponentBasicAnnotationsTest.java:83)

       

      testMethodParamsEmpty(org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderComponentBasicAnnotationsUnitTestCase)

      junit.framework.AssertionFailedError: null

      at junit.framework.Assert.fail(Assert.java:47)

      at junit.framework.Assert.assertTrue(Assert.java:20)

      at junit.framework.Assert.assertNotNull(Assert.java:214)

      at junit.framework.Assert.assertNotNull(Assert.java:207)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testEmpty(ComponentBasicAnnotationsTest.java:102)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testMethodParamsEmpty(ComponentBasicAnnotationsTest.java:90)

       

      testConstructorParamsEmpty(org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderComponentBasicAnnotationsUnitTestCase)

      junit.framework.AssertionFailedError: null

      at junit.framework.Assert.fail(Assert.java:47)

      at junit.framework.Assert.assertTrue(Assert.java:20)

      at junit.framework.Assert.assertNotNull(Assert.java:214)

      at junit.framework.Assert.assertNotNull(Assert.java:207)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testEmpty(ComponentBasicAnnotationsTest.java:102)

      at org.jboss.test.metadata.shared.ComponentBasicAnnotationsTest.testConstructorParamsEmpty(ComponentBasicAnnotationsTest.java:97)

       

      org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderNotPublicUnitTestCase

      testSameName(org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderNotPublicUnitTestCase)

      junit.framework.AssertionFailedError: null

      at junit.framework.Assert.fail(Assert.java:47)

      at junit.framework.Assert.assertTrue(Assert.java:20)

      at junit.framework.Assert.assertNotNull(Assert.java:214)

      at junit.framework.Assert.assertNotNull(Assert.java:207)

      at org.jboss.test.metadata.AbstractMetaDataTest.assertNoAnnotation(AbstractMetaDataTest.java:226)

      at org.jboss.test.metadata.loader.reflection.test.AnnotatedElementLoaderNotPublicUnitTestCase.testSameName(AnnotatedElementLoaderNotPublicUnitTestCase.java:94)

       

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

      The question is do I "fix" those tests or not? ComponentBasicAnnotationsTest comes in two varieties, both of which run this test:

       

         public void testFieldEmpty() throws Exception
         {
            MetaData metaData = setupField();
            metaData = metaData.getComponentMetaData(new FieldSignature("empty"));
            testEmpty(metaData);
         }
       
      

       

      • AnnotatedElementLoaderComponentBasicAnnotationsUnitTestCase which sets up the field "empty" as follows (FieldBean.empty has no annotations)
         protected MetaData setupField()
         {
            AnnotatedElementMetaDataLoader loader = new AnnotatedElementMetaDataLoader(FieldBean.class);
            return new MetaDataRetrievalToMetaDataBridge(loader);
         }
      
      •  

      MemoryLoaderComponentBasicAnnotationsUnitTestCase which sets up the "empty" field like this, which makes sure that there is a componentmetadata for it

         protected MetaData setupField()
         {
            MemoryMetaDataLoader loader = new MemoryMetaDataLoader();
            MemoryMetaDataLoader component = new MemoryMetaDataLoader();
            loader.addComponentMetaDataRetrieval(new FieldSignature("empty"), component);
            //Some more component MetaDataLoaders set up to test other "fields"
            ...
      
            return new MetaDataRetrievalToMetaDataBridge(loader);
         }
      

       

       

      We're clearly not counting on there always being a metadata. In practice if there is no component metadata for a particular signature, MetaDataLoader will return null while AnnotatedElementLoader will not, meaning we have two different behaviours for the implemented interface. On the other hand, in the real world (read AS kernel) with things as they have been the component metadata returned to the user will never be null since there would always at least be a created component AnnotatedElementMetaDataLoader there regardless of if all the component MemoryMetaDataLoaders were null or not.

        • 1. Re: AnnotatedElementMetaDataLoader component metadata optimization
          kabirkhan

          An alternative to changing the tests (although I think we should agree once and for all what the getComponentMetadataRetrieval() should return) is to return a singleton:

           

             public MetaDataRetrieval getComponentMetaDataRetrieval(Signature signature)
             {
                if (signature == null)
                   return null;
          
                if (annotated instanceof Class)
                {
                   Class<?> clazz = Class.class.cast(annotated);
                   if (signature instanceof ConstructorSignature)
                   {
                      ConstructorSignature constructorSignature = (ConstructorSignature) signature;
                      Constructor<?> constructor = constructorSignature.getConstructor();
                      if (constructor == null)
                         constructor = SecurityActions.findConstructor(clazz, signature.getParametersTypes(clazz));
                      if (constructor == null)
                      {
                         if (log.isTraceEnabled())
                            log.trace("Constructor with signature " + signature + " does not exist on class " + clazz.getName());
                         return null;
                      }
                      if (constructor.getAnnotations().length == 0)
                         return NullAnnotatedElementMetaDataLoader.INSTANCE;
                      return new AnnotatedElementMetaDataLoader(constructor);
                   }
                   //Same for fields, methods, parameters
             }
           
             private static class NullAnnotatedElementMetaDataLoader implements MetaDataRetrieval
             {
                final static NullAnnotatedElementMetaDataLoader INSTANCE = new NullAnnotatedElementMetaDataLoader();
                final static ScopeKey NULL_SCOPE_KEY = new ScopeKey(new Scope(CommonLevels.JOINPOINT, INSTANCE));
                final static BasicAnnotationsItem NO_ANNOTATIONS_ITEM = new BasicAnnotationsItem(null, new AnnotationItem[0]);
                final static BasicMetaDatasItem NO_METADATAS_ITEM = new BasicMetaDatasItem(null, new MetaDataItem[0]);
                
                public MetaDataRetrieval getComponentMetaDataRetrieval(Signature signature)
                {
                   return INSTANCE;
                }
          
                public ScopeKey getScope()
                {
                   return NULL_SCOPE_KEY;
                }
          
                public MetaDataRetrieval getScopedRetrieval(ScopeLevel level)
                {
                   return INSTANCE;
                }
          
                public ValidTime getValidTime()
                {
                   return null;
                }
          
                public boolean isEmpty()
                {
                   return true;
                }
          
                public <T extends Annotation> AnnotationItem<T> retrieveAnnotation(Class<T> annotationType)
                {
                   return null;
                }
          
                public AnnotationsItem retrieveAnnotations()
                {
                   return NO_ANNOTATIONS_ITEM;
                }
          
                public AnnotationsItem retrieveAnnotationsAnnotatedWith(Class<? extends Annotation> meta)
                {
                   return NO_ANNOTATIONS_ITEM;
                }
          
                public AnnotationsItem retrieveLocalAnnotations()
                {
                   return NO_ANNOTATIONS_ITEM;
                }
          
                public MetaDatasItem retrieveLocalMetaData()
                {
                   return NO_METADATAS_ITEM;
                }
          
                public MetaDatasItem retrieveMetaData()
                {
                   return NO_METADATAS_ITEM;
                }
          
                public <T> MetaDataItem<T> retrieveMetaData(Class<T> type)
                {
                   return null;
                }
          
                public MetaDataItem<?> retrieveMetaData(String name)
                {
                   return null;
                }
             }
          
           
          

          I am not sure about the ValidTime, and the ScopeKey seems to not be used, but all tests in mdr pass.

          • 2. Re: AnnotatedElementMetaDataLoader component metadata optimization
            kabirkhan

            Both of these fixes give problems when running the kernel tests.

             

            Returning null gives a few errors similar to the ones in MDR's tests, i.e. always expecting component metadata to be non-null in AnnotatedMethodTestCase and AnnotatedConstructorTestCase. This one line fix in AnnotatedMethodTestCase (and similar in AnnotatedConstructorTestCase) solves the problem and all the tests in kernel modules pass.

             

               private Annotation[] getComponentMetaData(KernelControllerContext context, Signature signature)
               {
                  KernelMetaDataRepository repository = context.getKernel().getMetaDataRepository();
                  MetaData metaData = repository.getMetaData(context);
                  MetaData component = metaData.getComponentMetaData(signature);
                  if (component == null)         //Added
                     return new Annotation[0];   //Added
                  return component.getAnnotations();
               }
            

             

             

            Returning NullAnnotatedElementMetaDataLoader.INSTANCE gives loads of errors in the jboss-kernel project, but they might be due to my NullAEMDL making some wrong assumptions, so I'll have a look at that.

            • 3. Re: AnnotatedElementMetaDataLoader component metadata optimization
              alesj

              Returning NullAnnotatedElementMetaDataLoader.INSTANCE gives loads of errors in the jboss-kernel project, but they might be due to my NullAEMDL making some wrong assumptions, so I'll have a look at that.

              I would go with NAEMDL.

              You need to check for emtpy annotations anyway,

              so there is no overhead if instead of null you return NAEMDL.

               

              Btw: NAEMLD doesn't need to have its variables static -- it will be singleton.

              • 4. Re: AnnotatedElementMetaDataLoader component metadata optimization
                kabirkhan

                Ok, I'll go with the singleton. The problems I was seeing with returning the singleton was that its getValidTime() method returned null, adding a validTime field makes all tests in mdr and kernel pass. I'll add a simple test for that and commit what I have so far against https://jira.jboss.org/jira/browse/JBMDR-66

                 

                So far we avoid creating AnnotatedElementLoaders and ScopeKeys for the cases where a member does not have any annotations.

                 

                I think this could be taken a bit further, when a member has annotations, we currently create an AnnotatedElementLoader and thus a ScopeKey. As mentioned ScopeKeys are expensive to create due to creating their contained ConcurrentSkipListMap and this add call in the constructor which adds to the ConcurrentSkipListMap:

                   public ScopeKey(Scope scope)
                   {
                      addScope(scope);
                   }
                
                Since it looks like there will only be one scope level in the key created by AnnotatedElementLoader, I am going to play with changing
                   private static final ScopeKey getScopeKey(AnnotatedElement annotated)
                   {
                      Scope scope;
                      if (annotated instanceof Class)
                      {
                         Class<?> clazz = Class.class.cast(annotated);
                         scope = new Scope(CommonLevels.CLASS, clazz);
                      }
                      else if (annotated instanceof Member)
                      {
                         Member member = (Member) annotated;
                         scope = new Scope(CommonLevels.JOINPOINT, member);
                      }
                      else
                      {
                         return ScopeKey.DEFAULT_SCOPE;
                      }
                      return new ScopeKey(scope);
                   }
                

                 

                to

                 

                   private static final ScopeKey getScopeKey(AnnotatedElement annotated)
                   {
                      Scope scope;
                      if (annotated instanceof Class)
                      {
                         Class<?> clazz = Class.class.cast(annotated);
                         scope = new Scope(CommonLevels.CLASS, clazz);
                         return new ScopeKey(scope);
                      }
                      else if (annotated instanceof Member)
                      {
                         Member member = (Member) annotated;
                         scope = new Scope(CommonLevels.JOINPOINT, member);
                         return new SingleLevelScopeKey(scope);
                      }
                      else
                      {
                         return ScopeKey.DEFAULT_SCOPE;
                      }
                   }
                
                 
                

                 

                where SingelLevelScopeKey will be a lightweight implementation of ScopeKey that does not have the internal ConcurrentSkipListMap. I might even be able to use the existing UnmodifiableScopeKey, so I will look into that first.

                 

                I think this could this also be used for the CLASS scope?

                 

                I need to check, but will see if this could this also be used for the other metadata retrievals.

                • 5. Re: AnnotatedElementMetaDataLoader component metadata optimization
                  kabirkhan

                  I changed it to use UnmodifiableScopeKey for both CLASS and JOINPOINT levels.

                   

                  When creating UnmodifiableScopeKey, there was still some overhead in unnecessarily creating the parent ScopeKey.scopes map which will always be ignored, so I have made that lazily initialized in ScopeKey.addScope(Scope).

                  • 6. Re: AnnotatedElementMetaDataLoader component metadata optimization
                    kabirkhan

                    I did some AS starts with my initial implementation (AnnotatedElementMetaDataLoader.getComponentMetaDataRetrieval() returns null if no annotations) from this thread yesterday, and when rerunning them today and comparing with another AS instance with the alternative solution and rest of the fixes from this thread I found that JBoss AS with today's jboss-mdr snapshot starts up slower than yesterday's jboss-mdr snapshot by almost a second. I'll look into why.

                    • 7. Re: AnnotatedElementMetaDataLoader component metadata optimization
                      kabirkhan

                      I have done some benchmarks for the different solutions for AnnotatedElementMetaDataLoader.getComponentMetaDataRetrieval().

                       

                      1. The worst performing is the original form, creating a new AEMDL when there are no annotations
                      2. The best performing is returning null when there are no annotations, taking half the time of 1
                      3. What we have now performs somewhere in between, about 1.5 times that of 2

                       

                      In all cases the use of CachingMetaDataContext (which I create outside the benchmark, so this is for reads only) performs worse than the plain MetaDataContext, but I think it is needed for getAnnotationsAnnotatedWith()?

                      • 8. Re: AnnotatedElementMetaDataLoader component metadata optimization
                        kabirkhan

                        I have gone back to making AEMDL.getCMDR() return null if there are no annotations. It is the fastest way, and is the least affected by caching. To preserve backwards compatibility and to not have to change any tests, I have added this fix to make MetaDataRetrievalToMetaDataBridge always return something even if there is no component metadata:

                         

                           public MetaData getComponentMetaData(Signature signature)
                           {
                              MetaDataRetrieval component = retrieval.getComponentMetaDataRetrieval(signature);
                              if (component == null)
                                 //return null;
                                 return NullComponentMetaData.INSTANCE;
                              return new MetaDataRetrievalToMetaDataBridge(component);
                           }
                        ...
                           private static class NullComponentMetaData implements MetaData
                           {
                              final static NullComponentMetaData INSTANCE = new NullComponentMetaData();
                        
                              public <T extends Annotation> T getAnnotation(Class<T> annotationType)
                              {
                                 return null;
                              }
                        
                              public Annotation[] getAnnotations()
                              {
                                 return MetaData.NO_ANNOTATIONS;
                              }
                        
                              public Annotation[] getAnnotationsAnnotatedWith(Class<? extends Annotation> meta)
                              {
                                 return MetaData.NO_ANNOTATIONS;
                              }
                        
                              public MetaData getComponentMetaData(Signature signature)
                              {
                                 return null;
                              }
                        
                              public Annotation[] getLocalAnnotations()
                              {
                                 return MetaData.NO_ANNOTATIONS;
                              }
                        
                              public Object[] getLocalMetaData()
                              {
                                 return MetaData.NO_METADATA;
                              }
                        
                              public <T> T getMetaData(Class<T> type)
                              {
                                 return null;
                              }
                        
                              public Object[] getMetaData()
                              {
                                 return MetaData.NO_METADATA;
                              }
                        
                              public Object getMetaData(String name)
                              {
                                 return null;
                              }
                        
                              public <T> T getMetaData(String name, Class<T> type)
                              {
                                 return null;
                              }
                        
                              public MetaData getScopeMetaData(ScopeLevel level)
                              {
                                 return null;
                              }
                        
                              public long getValidTime()
                              {
                                 return 0;
                              }
                        
                              public boolean isAnnotationPresent(Class<? extends Annotation> annotationType)
                              {
                                 return false;
                              }
                        
                              public boolean isEmpty()
                              {
                                 return true;
                              }
                        
                              public boolean isMetaDataPresent(Class<?> type)
                              {
                                 return false;
                              }
                        
                              public boolean isMetaDataPresent(String name)
                              {
                                 return false;
                              }
                        
                              public boolean isMetaDataPresent(String name, Class<?> type)
                              {
                                 return false;
                              }  
                           }