Avoid unnecessary locking in ClassInfo.getMetaClass

If I look at the locking I see that it is done on a per ClassInfo instance base. That means it is possible to get multiple meta classes at the same time even with this locking. The reason why it is a "stop the world lock" for you is probably that all your threads go the boolean#asType path. That means all many of your threads request one and the same ClassInfo instance to then block each other until they are served one-by-one.

A meta class change relevant for this locking can happen in two cases.
(1) the older instance was garbage collected (which cannot happen with a modified meta class)
(2) you changed the meta class manually

While you say you do (2) at the beginning we have to try to design the core to enable (2) all the time atm.

let us think about a Read-WriteLock.... what currently calls lock should do the write lock. Then we can use the internal code to do the read locks.

is probably that all your threads go the boolean#asType path. That means all many of your threads request one and the same ClassInfo instance to then block each other until they are served one-by-one.

That's right. We have lots of calls similar to the following:

translations.any { it.toLowerCase().contains(text) }

(sometimes even nested). I guess it will request getClassInfo(Boolean.class) inside a long loop.

From what I see in ClassInfo there are actually two different methods:

1. getMetaClass()
2. getMetaClass(Object obj) - checks if instance has an instance-level metaclass assigned, then locks.

InvokerHelper goes to the second one. Since objects rarely have instance-level metaclasses assigned, there's a high chance to get blocked here.

Serge, could you test if this patch makes a difference for you?

Hello. I've tried to patch groovy 1.7.10 and got the same problem.
I see your patch applies to ClassInfo.getMetaClass() while the most critical execution path is ClassInfo.getMetaClass(obj).

Sorry for the mistake, here is the correct version

Did the fixed patch work? Is it going to be included in a future version?

Hi, we've upgraded to Grails 2.0 recently, which resolved some of our issues. I will try to squeeze the patch into Grails 2.0 production (it's somewhat tricky) and see how it affects our performance graphs. This will take about a week.

FYI, Netflix's internal Grails-based tooling is running into this bug pretty often now. We're rewriting the worst bottleneck parts of our code as needed until the fix comes out. Mostly we're reducing use of find() or any() or findAll() in frequently-called nested closures or nested iterations.

Joe, Serge seems not to have the time to test it yet. Can you test the patch?

Hi, I work with Joe at Netflix. We're no longer seeing this problems as frequently after tweaking a few places where we were doing nested find() calls while iterating over a large collection.

I'll try to test the patch if I get a chance. Since it's somewhat tricky to reproduce the problems, it might be a while before I get to it. Prior to those fixes, this problem was causing our Grails app to become unresponsive almost daily.

There is a thread on Grails mailing list that has interesting findings of performance of calling private helper methods in a closure:
http://grails.1312388.n4.nabble.com/Thread-Blocking-at-Groovy-ExpandoMetaclass-method-in-a-Grails-App-tp4572990p4574077.html
(the FilterConfig performance problem is a grails problem, but the later introduced StoryDao example is interesting from Groovy performance point of view)

Surprisingly a workaround is to call the helper method on "thisObject" prefix. Based on the stacktrace this seems to be some problem in ExpandoMetaClass which is enabled globally in Grails. There seems to be a bug in ExpandoMetaClass's addSuperMethodIfNotOverridden . It only handles ClosureMetaMethod instances.

Joe & Jason, GROOVY-5428 explains the bad performance of find(), any() or findAll() in certain cases. Have you monitored if a large number of Exceptions (ClassCastExceptions) are created?

We've managed to test a patch, trying it about 2 weeks in production, using Grails 2.0.3 + Groovy 1.8.6 + patch.
Two things to mention:

1. Number of busy threads sometimes lowered a bit, but not significantly.
2. No difference in CPU usage looking at graphs...

So the patch does, in fact, work. However in our case its effect is fairly minor compared to Grails + Groovy (1.8.x) upgrade.
I think GROOVY-5059 actually had more impact considering how our code is written.

I encountered performance issue caused by locking in ClassInfo.getMetaClass with groovy 1.8.7 when using following construction concurrently:

            switch (node.name()) {
                case ['b','s','p','para','sect']:
...

Stacktrace is following:

	  at sun.misc.Unsafe.park(Unsafe.java:-1)
	  at java.util.concurrent.locks.LockSupport.park(LockSupport.java:156)
	  at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:811)
	  at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:842)
	  at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1178)
	  at org.codehaus.groovy.util.LockableObject.lock(LockableObject.java:34)
	  at org.codehaus.groovy.reflection.ClassInfo.lock(ClassInfo.java:268)
	  at org.codehaus.groovy.reflection.ClassInfo.getMetaClass(ClassInfo.java:193)
	  at org.codehaus.groovy.runtime.metaclass.MetaClassRegistryImpl.getMetaClass(MetaClassRegistryImpl.java:231)
	  at org.codehaus.groovy.runtime.InvokerHelper.getMetaClass(InvokerHelper.java:747)
	  at org.codehaus.groovy.runtime.InvokerHelper.invokePojoMethod(InvokerHelper.java:780)
	  at org.codehaus.groovy.runtime.InvokerHelper.invokeMethod(InvokerHelper.java:772)
	  at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.invokeMethodN(ScriptBytecodeAdapter.java:164)
	  at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.isCase(ScriptBytecodeAdapter.java:667)

invokeMethodN(caseExpression.getClass(), caseExpression, "isCase", new Object[]{switchValue})

is locked on ClassInfo for ArrayList, because it uses invokePojoMethod which calls ClassInfo.getMetaClass(obj).

What I get out of this thread is that we have a general problem with the performance of Groovy-truth from Java code. As a method call from Groovy it would be not such a big problem, since the method would be cached, and not every call will go through getMetaClass in ClassInfo. But in this last trace for example isCase does this, since it is an uncached dynamic call

has someone reading in this thread any idea how to prevent that? I could apply some logic from the primitive optimizations, but EMC is disabling that. I could try hand made call site caching in the Java code... that would help in some cases maybe... in JDK 1.7 I could leverage ClassValue and avoid the locking probably... But that doesn't help in Groovy 1.8 and not on pre jdk7 vms

I added several changes that should help with the usage of Groovy Truth in internal iterating methods such as any, find and grep. The changes I did will not help with the switch-case Timur reported. What I did was adding a caching for the meta methods involved, especially Closure#call and asBoolean. The logic for Groovy-Truth is unchanged, only faster I hope

I guess for the switch-case we need to think a bit more about what to do. But for example Serge should see an inprovement.

This is also a problem in Grails 2.2.0 / Groovy 2.0.5 . See Grails mailing list thread for more details: http://grails.1312388.n4.nabble.com/Grails-performance-restriction-tt4642061.html . It looks like InvokerHelper.getProperty triggers this problem when POJOs are used. Making POJOs extend GroovyObjectSupport (or implement GroovyObject) is one workaround for this problem before this issue gets fixed.

Graeme suggested another workaround in the Grails mailing list thread: "GroovySystem.keepJavaMetaClasses = true" setting might help. @CompileStatic does too.