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

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.

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.

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.

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.

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?

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 {
    
}

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.

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.

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.

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.

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

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.

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.

fixed with the instanceof check only