Remove synchronization of isModified method in ExpandoMetaClass to improve performance

Isn't this a Groovy scalability issue if ExpandoMetaClass.isModified() causes the performance issue?

I tried changing taglib beans to request scope and that was very easy (a couple of changes to GroovyPagesGrailsPlugin and TagLibraryLookup). Currently I get an error "Scope 'request' is not active for the current thread" (fails in GroovyPagesGrailsPlugin's doWithDynamicMethods). I was thinking of adding a FactoryBean class that does that. Parts from GroovyPagesGrailsPlugin's doWithDynamicMethods could be moved to that FactoryBean that set's up a new taglib bean instance (and it's metaclass). The problem is that this would probably break existing plugins. I assume plugins might want to change the taglib's metaclass too.

That was our initial thought, and I guess that could be argued, but perhaps it could also be argued that good multi-threaded performance with Groovy depends on avoiding singletons.

As TagLibs are singletons and make use of the Groovy meta classes, Grails is affected by this issue.

If you can get it working with the FactoryBean, then it would be good to confirm that this reduces the locking and addresses the scalability problems we have experienced.

If it does fix this, then we could look at how to address the other issues?

Just found this old issue: GRAILS-1036 , might be interesting. Relates to GROOVY-1826 .

I got request scope working for taglib beans. This patch is against the changes I have in my gspcompile branch in my grails fork (http://github.com/lhotari/grails/tree/gspcompile). The patch might not merge directly to the master-branch of grails but you should be able to see what has changed.
The metaclass is still shared so it probably doesn't reduce synchronization/locking.

I think ExpandoMetaClass has to be optimized for performance. It should be possible to reduce locking by using a different pattern for notifying other threads about changes.

I'm not sure if it helps (or if it breaks the usage) but I was thinking of changing "boolean modified" to "volatile boolean modified" and removing synchronized from isModified() method. Similar optimization might be necessary for initialized. These changes can cause thread-safety problems, but at least we could test if these increase performance (if it does, then there is a performance problem in ExpandoMetaClass). Mark, can you make this kind of change to ExpandoMetaClass and re-run your tests.

I tested a custom version of ExpandoMetaClass which had volatile modifier added for modified and initialized fields and the synchronizations removed. This gave a 7% performance increase in a micro-benchmark with 100 concurrent threads each running for 1 million times. The performance increase can be slightly higher since I calculated the improvement from the micro benchmark's total execution time (jvm's jit compilation time gets included). Anyhow I don't think that ExpandoMetaClass is a major performance problem for taglibs, but it shows that there is a lot of parts in groovy and grails that can be performance optimized in the future.

I have rerun our tests with the standard ExpandoMetaClass and one with a volatile modifier and no synchronization on isModified.

I can see no significant difference on the performance of each version. In both cases we see throughput level out at around 400 requests / min at 50 concurrent clients. FYI our test box is a Dual Xeon, so 4 cores + Hyperthreading.

We have however modified our application to use more taglibs and to be careful about accessing the dynamic properties of the Taglib (out, request, response etc.). This may have made the synchronisation to no longer be the factor limiting the performance of our application.

I will roll back our application to the state before these changes were made and re-run the tests - hopefully tomorrow..

What we have been seeing in the profiler is that when blocking occurs on the isModified Method, the "Wall Time" see http://www.yourkit.com/docs/75/help/times.jsp for the blocked threads is quite high.

I suspect that this is due to the JVM doing a lot of switching being threads - the fact that the method is called a large number of times does not help this situation. When there are 100+ "active" requests and (only) 4 real (+4 HT) cores, it can take some time to get back to the thread that was blocked.

Anyway using volatile and removing the synchronisation removes the blocking issues we have been encountering - but this has led to us now experiencing problems with running out of database connections in the pool, so the performance increase we are seeing (5-10%) is probably not an accurate indication of the possible increase.

Subject to there not being any other side effects, making this modification to Groovy would be of benefit. I am not a threading expert, but from my understanding don't think that using volatile rather than synchronized for a boolean would not cause issues. I also note that the methods that change the value of this member in ExpandoMetaClass do not have any synchronization anyway.

Moving to Groovy so the change can be done there

the access to the modified flag is maybe not synchronized, but such changes are supposed to be done using performOperationOnMetaClass, which does synchronize on "this", just as isModified is doing. So the access is actually synchronized if it is going through that method, which should normally be the case.

But I agree that making it volatile and removing the synchronized flag from isModified() should do the job.

The attached patch I have to reject though. It is changing taglib stuff, which is Grails,and does not contain a single change to EMC. I assume Mark just wanted to show what he did for scoped tags

btw. There's also a synchronization on isInitialized. I think isInitialized gets called in every call too (like isModified).
Maybe the synchronization should be removed for initialized too (use a volatile field instead).

I reduced the priority to "major", because this is not really a critical bug. Also it is no real bug. because it just works. So I changed it into "improvement".

As for isInitialized()... true, same story, same fix

I changed synchronized to volatile for modifed and initialized. In my micro benchmark this improved method call performance up to factor 3, which is certainly not what I expected.

This change might be causing problems in Grails 1.2-M4, see GRAILS-5345 .

Re-opening because of GRAILS-5345, the consensus right now is to rollback the change for Groovy 1.6.6

I reverted the change now

This hasn't been rolled back, i see no evidence of the rollback occuring in commit log at http://svn.groovy.codehaus.org/changelog/groovy/branches/GROOVY_1_6_X?max=30&maxDate=1256928484689&prevPageAnchor=changeset%3A18170&view=all

now really fixed

I'm going to re-open this issue since the change was rolled back but the underlying performance problem is still there and we need to find a solution for it since it is a huge performance bottleneck in highly concurrent applications that use EMC.

I understand there is no time to look at this for 1.8 but for one of the point releases we should try and find a solution.

EMC actually has several modes:

(1) init

During init we populate the EMC with the normal methods of the class and our additional methods. The problem of this mode is, that if an method invocation from another thread is done while EMC is in this mode, the current logic requires that method call to wait till init is done. That is the main reason for the synchronized blocks used in EMC.

The waiting requirement could be reduced in that we say creating a new EMC will right away init the EMC, then you will get back a ready initialized EMC instead of having to wait for the initialize call to finnish. But this is a bigger semantic change I think an we have to ensure this is no problem for Grails.

(2) modification

During modification we change EMC and during this modification all method invocations are blocked like in init. Actually this mode currently kind of uses a modified init mode.

I think we could here maybe work with some copy-on-write semantics to not to block other method calls during modification. Of course that again means a change of the multi threaded behaviour of this class and people depending on the blocking nature of the operation would possibly get a problem.

(3) execution

In that mode you can get method or execute them concurrently. The problem in this mode is the check we need for the init flag. The modified flag is no longer synchronized or volatile, so this is no problem anymore I would assume.

If it is possible to change modes 1+2 like I described, then we can remove the init flag completely, thus it cannot block anymore.

But... is that a too big change for 1.8?

my first try on implementing that for 1.9 quite failed. MetaMethodIndex is a big complex, highly mutable and highly integrated structure, that is no fun to replace, especially because of its tendency to appear here and there

Pull request for a fix for this problem: https://github.com/groovy/groovy-core/pull/3
Very simple change, uses ReentrantReadWriteLock for R/W locking.

I merged the pull request.

Sorry, this ticket is closed but I don't think it really fixes the problem with locking.

Just a note, lots of Grails code flows through InvokerHelper which results in the following stack:

"http-apr-8080"-exec-144
sun.misc.Unsafe.park(Native Method)
java.util.concurrent.locks.LockSupport.park(LockSupport.java:158)
java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:811)
java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:842)
java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1178)
org.codehaus.groovy.util.LockableObject.lock(LockableObject.java:34)
org.codehaus.groovy.reflection.ClassInfo.lock(ClassInfo.java:268)
org.codehaus.groovy.reflection.ClassInfo.getMetaClass(ClassInfo.java:193)
org.codehaus.groovy.runtime.metaclass.MetaClassRegistryImpl.getMetaClass(MetaClassRegistryImpl.java:214)
org.codehaus.groovy.runtime.InvokerHelper.getMetaClass(InvokerHelper.java:747)
org.codehaus.groovy.runtime.InvokerHelper.invokePojoMethod(InvokerHelper.java:780)
org.codehaus.groovy.runtime.InvokerHelper.invokeMethod(InvokerHelper.java:772)
org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation.castToBoolean(DefaultTypeTransformation.java:156)
org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation.booleanUnbox(DefaultTypeTransformation.java:65)

We have a Grails application serving hundreds of requests per second and this seems to be the most critical hot spot. As you can see, locking occurs in ClassInfo.getMetaClass, probably THAT is the right place for using ReadWriterLock.

The issue was about isModified synchronization in EMC, this is done. What you talk about is a new issue, so please make a new issue then.

Serge, your stacktrace looks like GROOVY-5059 problem. It's fixed in Groovy 1.8.4 (and Grails 2.0).

Lari, yes and no. AFAIK GROOVY-5059 is an improvement for boolean casts where source object is itself Boolean. When source object is not Boolean, it has to extract MetaClass anyway, invoking lock() and then getMetaClassUnderLock(). Also there are many other cases where InvokerHelper is used. For example, I think the following loop is not going to be improved by GROOVY-5059:

['1','2',3'].any{!it}

.

Since it's a different issue I've created GROOVY-5249 hoping that someone can take a look...

Serge, agree. I should have said "improved in Groovy 1.8.4" instead of "fixed".