groovy
  1. groovy
  2. GROOVY-5428

Performance problem: ClassCastExceptions are created in normal execution flow which kills performance

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.6
    • Fix Version/s: 2.0-beta-3, 1.8.7
    • Component/s: None
    • Labels:
      None
    • Environment:
      Grails 2.0.3 + Groovy 1.8.6
    • Number of attachments :
      3

      Description

      This problem is a major performance bottleneck in Grails 2.0 applications.
      Currently this problem shows up when resources plugin has been installed and used in a Grails app. (see attached screenshot)

      Exceptions are relatively heavy weight in Java. Filling the stacktrace takes most of the time. Normal execution flow should never throw exceptions.
      (it is possible to disable filling the stacktrace like has been done for MissingMethodExceptionNoStack, in that case the performance overhead is usually acceptable.)

      The problem shows up whenever there is 2 classes sharing the same base class and when the other class is a Java class and the other one is a Groovy class (GroovyObject).

      I was able to reproduce the problem with this code. Run this code in a debugger and make a breakpoint for the construction of ClassCastException.
      In this example the base class is java.util.Map , the Java class is LinkedHashMap and the Groovy class is MyMap (extends LinkedHashMap).

      class ClassCastProblem {
          def doCalls() {
              def m1=[abc:'test']
              def m2=new MyMap()
              doSomething(m1)
              doSomething(m2)
              doSomething(m1)
              doSomething(m2)
              doSomething(m1)
          }
          
          void doSomething(m) {
              m.remove('test')
          }
      
          public static void main(String[] args) {
              def ccp=new ClassCastProblem()
              ccp.doCalls()
          }
      }
      
      class MyMap extends LinkedHashMap {
          
      }
      

      In a Grails app the 2 different java.util.Map implementations usually triggering this problem are LinkedHashMap and GrailsParameterMap (GroovyObject).

      In Grails core I've worked around some of the problem with this type of hack:
      https://github.com/grails/grails-core/commit/62626921ebd80e3cdeb9776fd79be7eb92f88763
      At that time I didn't have time to investigate the reason that caused the problem and just made some changes until the problem went away.

        Activity

        Hide
        Lari Hotari added a comment -

        Example with Lists & find:

        class ClassCastProblem2 {
            def doCalls() {
                def l1=['a','b','c']
                def l2=new MyList()
                doSomething(l1)
                doSomething(l2)
                doSomething(l1)
                doSomething(l2)
                doSomething(l1)
            }
            
            void doSomething(l) {
                l.find { 
                    it
                }
            }
        
            public static void main(String[] args) {
                def ccp=new ClassCastProblem2()
                ccp.doCalls()
                println('done')
            }
        }
        
        class MyList extends ArrayList {
            
        }
        

        The problem shows up also with DGM methods.

        Show
        Lari Hotari added a comment - Example with Lists & find: class ClassCastProblem2 { def doCalls() { def l1=['a','b','c'] def l2= new MyList() doSomething(l1) doSomething(l2) doSomething(l1) doSomething(l2) doSomething(l1) } void doSomething(l) { l.find { it } } public static void main( String [] args) { def ccp= new ClassCastProblem2() ccp.doCalls() println('done') } } class MyList extends ArrayList { } The problem shows up also with DGM methods.
        Hide
        blackdrag blackdrag added a comment -

        The code in the checkCall method more or less is that:

                    return usage.get() == 0
                       && ((GroovyObject)receiver).getMetaClass() == metaClass // metaClass still be valid
                       && MetaClassHelper.sameClasses(params, args); 

        If that is causing a ClassCastException, then because receiver is no longer a GroovyObject. In cases in which the receiver does not change all the time from Groovy class to Java class and back, this code is surely better than having and additional instanceof in there. With a more sophisticated caching we could recognize this case and add such a check only in the all so often changing case, maybe even keeping the old callsite and switching between those two. That would surely improve performance. But the caching does not give that kind of logic. If we include the check all method invocations based on GroovyObject will be slower, but that is not all. Even with the check your program has a problem, since it will cause the creation of a new call site every time. What I am trying to tell you is, that even if we add that check the performance of that program will stay bad. The Call site generation takes much more time than the ClassCastException.

        Show
        blackdrag blackdrag added a comment - The code in the checkCall method more or less is that: return usage.get() == 0 && ((GroovyObject)receiver).getMetaClass() == metaClass // metaClass still be valid && MetaClassHelper.sameClasses(params, args); If that is causing a ClassCastException, then because receiver is no longer a GroovyObject. In cases in which the receiver does not change all the time from Groovy class to Java class and back, this code is surely better than having and additional instanceof in there. With a more sophisticated caching we could recognize this case and add such a check only in the all so often changing case, maybe even keeping the old callsite and switching between those two. That would surely improve performance. But the caching does not give that kind of logic. If we include the check all method invocations based on GroovyObject will be slower, but that is not all. Even with the check your program has a problem, since it will cause the creation of a new call site every time. What I am trying to tell you is, that even if we add that check the performance of that program will stay bad. The Call site generation takes much more time than the ClassCastException.
        Hide
        Lari Hotari added a comment -

        I've previously provided 2 code examples where the receiver changes from Groovy class to Java class and back.

        This happens very often in Grails applications. Usually it happens in Grails with java.util.Map methods (Map implementations LinkedHashMap, GroovyPageAttributes and GrailsParameterMap). The popular Grails resources plugin reproduces this problem too.

        The Call site generation takes much more time than the ClassCastException.

        So the call site generation might be even a larger bottleneck? I hope this issue gets fixed asap since it affects all Grails 2.0 applications that use the resources plugin.

        Show
        Lari Hotari added a comment - I've previously provided 2 code examples where the receiver changes from Groovy class to Java class and back. This happens very often in Grails applications. Usually it happens in Grails with java.util.Map methods (Map implementations LinkedHashMap, GroovyPageAttributes and GrailsParameterMap). The popular Grails resources plugin reproduces this problem too. The Call site generation takes much more time than the ClassCastException. So the call site generation might be even a larger bottleneck? I hope this issue gets fixed asap since it affects all Grails 2.0 applications that use the resources plugin.
        Hide
        blackdrag blackdrag added a comment -

        call site generation means a certain overhead, if you do it all the time, then it becomes a performance problem. The problem is, there is no fast fix for this issue.

        Show
        blackdrag blackdrag added a comment - call site generation means a certain overhead, if you do it all the time, then it becomes a performance problem. The problem is, there is no fast fix for this issue.
        Hide
        Lari Hotari added a comment -

        I think we could eliminate the ClassCastException. That's causing the overhead in this case and that could be fixed. The call site generation is only instantiating a pre-generated call site class and I don't believe that's expensive. The old call site instance gets replaced also when one of the parameter class changes so that's business as usual.

        I'd replace "((GroovyObject)receiver).getMetaClass() == metaClass" with "receiver.getClass() == metaClass.getTheClass()" (used in PojoMetaMethodSite). Do you think that's ok?

        Show
        Lari Hotari added a comment - I think we could eliminate the ClassCastException. That's causing the overhead in this case and that could be fixed. The call site generation is only instantiating a pre-generated call site class and I don't believe that's expensive. The old call site instance gets replaced also when one of the parameter class changes so that's business as usual. I'd replace "((GroovyObject)receiver).getMetaClass() == metaClass" with "receiver.getClass() == metaClass.getTheClass()" (used in PojoMetaMethodSite). Do you think that's ok?
        Hide
        Lari Hotari added a comment -

        This isn't a major performance issue. I did some benchmarking.

        
        class ClassCastProblem {
            def doCalls() {
                def m1=[abc:'test']
                def m2=new MyMap()
        
        	println "Warmup..."
                for(int i=0;i < 100000;i++) {	
                   doSomething(m1)
                   doSomething(m2)
        	}
        	println "Starting..."
        	long start=System.currentTimeMillis()
        	for(int i=0;i < 10000000;i++) {
                   doSomething(m1)
                   doSomething(m2)
        	   if(i%1000==0) print "."
                }
        	long duration=System.currentTimeMillis()-start
        	println "Duration ${duration} ms"
            }
            
            void doSomething(m) {
                m.remove('test')
            }
        
            public static void main(String[] args) {
                def ccp=new ClassCastProblem()
                ccp.doCalls()
            }
        }
        
        class MyMap extends LinkedHashMap {
            
        }
        
        Show
        Lari Hotari added a comment - This isn't a major performance issue. I did some benchmarking. class ClassCastProblem { def doCalls() { def m1=[abc:'test'] def m2= new MyMap() println "Warmup..." for ( int i=0;i < 100000;i++) { doSomething(m1) doSomething(m2) } println "Starting..." long start= System .currentTimeMillis() for ( int i=0;i < 10000000;i++) { doSomething(m1) doSomething(m2) if (i%1000==0) print "." } long duration= System .currentTimeMillis()-start println "Duration ${duration} ms" } void doSomething(m) { m.remove('test') } public static void main( String [] args) { def ccp= new ClassCastProblem() ccp.doCalls() } } class MyMap extends LinkedHashMap { }
        Hide
        blackdrag blackdrag added a comment -

        the generated call site is kept? I forgot that... and it does not go through method selection anymore then? Lari, as for your test... with what do you compare these numbers?

        Then about comparing the class instead of the meta class. The call site becomes invalid if the receiver meta class is not what we expect. GroovyObject means per instance meta classes are supported and that can mean any receiver can have a different meta class, even the same receiver at a later point. That's why ensuring the meta class is the same is a requirement.

        Show
        blackdrag blackdrag added a comment - the generated call site is kept? I forgot that... and it does not go through method selection anymore then? Lari, as for your test... with what do you compare these numbers? Then about comparing the class instead of the meta class. The call site becomes invalid if the receiver meta class is not what we expect. GroovyObject means per instance meta classes are supported and that can mean any receiver can have a different meta class, even the same receiver at a later point. That's why ensuring the meta class is the same is a requirement.
        Hide
        Lari Hotari added a comment -

        Yes, it's kept in CachedMethod:

        private SoftReference<Constructor> pogoCallSiteConstructor, pojoCallSiteConstructor, staticCallSiteConstructor;
        

        I benchmarked against a version of checkCall where I added "receiver.getClass() == metaClass.getTheClass()" to prevent the ClassCastException:

                      return !GroovyCategorySupport.hasCategoryInCurrentThread()
        +               && receiver.getClass() == metaClass.getTheClass()
                        && ((GroovyObject)receiver).getMetaClass() == metaClass // metaClass still be valid
                        && MetaClassHelper.sameClasses(params, args);
        

        The timings were 6500ms (1.8.6) compared to 5250ms (patched) so this is not a performance bottleneck . That's with 10 million iterations. It will improve performance, but it's not a big change.

        I'll also attach a patch if anyone wants to apply it.

        Show
        Lari Hotari added a comment - Yes, it's kept in CachedMethod: private SoftReference<Constructor> pogoCallSiteConstructor, pojoCallSiteConstructor, staticCallSiteConstructor; I benchmarked against a version of checkCall where I added "receiver.getClass() == metaClass.getTheClass()" to prevent the ClassCastException: return !GroovyCategorySupport.hasCategoryInCurrentThread() + && receiver.getClass() == metaClass.getTheClass() && ((GroovyObject)receiver).getMetaClass() == metaClass // metaClass still be valid && MetaClassHelper.sameClasses(params, args); The timings were 6500ms (1.8.6) compared to 5250ms (patched) so this is not a performance bottleneck . That's with 10 million iterations. It will improve performance, but it's not a big change. I'll also attach a patch if anyone wants to apply it.
        Hide
        Lari Hotari added a comment -

        The generated call sites can get GCed since it's a SoftReference in CachedMethod that keeps the reference of a generated call site. That might be a problem under memory pressure.

        Show
        Lari Hotari added a comment - The generated call sites can get GCed since it's a SoftReference in CachedMethod that keeps the reference of a generated call site. That might be a problem under memory pressure.
        Hide
        blackdrag blackdrag added a comment -

        The performance improvement will be much less if you replace the cast with an instanceof and cast, followed by getting the metaclass, instead of calling the intrinsic getClass() method.

        Even if the methods are cached, there is still the issue with having to go through method selecton every time. I was asking what you compare with, because the reference should be the case with cached method calls (ie only m2 and no m1). And there I expect like half the time needed, if not less.

        Show
        blackdrag blackdrag added a comment - The performance improvement will be much less if you replace the cast with an instanceof and cast, followed by getting the metaclass, instead of calling the intrinsic getClass() method. Even if the methods are cached, there is still the issue with having to go through method selecton every time. I was asking what you compare with, because the reference should be the case with cached method calls (ie only m2 and no m1). And there I expect like half the time needed, if not less.
        Hide
        blackdrag blackdrag added a comment -

        Lari, I added the instanceof check for now. Should we close the issue with that?

        Show
        blackdrag blackdrag added a comment - Lari, I added the instanceof check for now. Should we close the issue with that?
        Hide
        Lari Hotari added a comment -

        Yes it's ok to close this one.

        btw. I wonder if the SoftReference pogoCallSiteConstructor and pojoCallSiteConstructor fields of CachedMethod could get GCed under memory pressure and that would cause CachedMethod to generate a new callsite class (bytecode) for the same method (if the callsite gets swapped). I've never seen that happen but that possible problem came into mind.

        Show
        Lari Hotari added a comment - Yes it's ok to close this one. btw. I wonder if the SoftReference pogoCallSiteConstructor and pojoCallSiteConstructor fields of CachedMethod could get GCed under memory pressure and that would cause CachedMethod to generate a new callsite class (bytecode) for the same method (if the callsite gets swapped). I've never seen that happen but that possible problem came into mind.
        Hide
        blackdrag blackdrag added a comment -

        can they be GCed? well.... in a test like yours unlikely, because there is no or almost no object creation between the method calls and during the method call it surely will not happen, since the object is "in use". But after a while, or after lots of memory usage this can happen, yes. And yes, the swapped out call site is even more likely to be collected. But without the SoftReference the ChachedClass object would just become bigger and then we would not only have to create the call site again, we would also have to create the cached class again.

        Show
        blackdrag blackdrag added a comment - can they be GCed? well.... in a test like yours unlikely, because there is no or almost no object creation between the method calls and during the method call it surely will not happen, since the object is "in use". But after a while, or after lots of memory usage this can happen, yes. And yes, the swapped out call site is even more likely to be collected. But without the SoftReference the ChachedClass object would just become bigger and then we would not only have to create the call site again, we would also have to create the cached class again.
        Hide
        blackdrag blackdrag added a comment -

        fixed with the instanceof check only

        Show
        blackdrag blackdrag added a comment - fixed with the instanceof check only

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Lari Hotari
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: