groovy
  1. groovy
  2. GROOVY-3557

Remove synchronization of isModified method in ExpandoMetaClass to improve performance

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.6, 1.6.1, 1.5.8, 1.6.2, 1.6.3
    • Fix Version/s: 1.9-beta-4, 1.8.4
    • Component/s: None
    • Labels:
      None
    • Environment:
      Linux, Java 1.6, Tomcat
    • Number of attachments :
      1

      Description

      Sscalability seems to suffer as a result of TagLibs being implemented as singletons and locking occurring on ExpandoMetaClass.isModified() when the server is processing multiple requests concurrently.

      Performance is fine when there is a small number of concurrent requests, but as the number of concurrent requests increases response times increase dramatically.

      Locking was noticed by profiling a running application using Your Kit Java Profiler and JMeter to simulate load.

      More details are available here:
      http://digitalmorphosis.blogspot.com/2009/05/grails-scalability.html

      Obvious fix would be to create new instances of taglibs for each request/thread, but not sure what side effects this would have?

        Activity

        Hide
        Lari Hotari added a comment -

        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.

        Show
        Lari Hotari added a comment - 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.
        Hide
        Mark Smithson added a comment -

        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?

        Show
        Mark Smithson added a comment - 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?
        Hide
        Lari Hotari added a comment -

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

        Show
        Lari Hotari added a comment - Just found this old issue: GRAILS-1036 , might be interesting. Relates to GROOVY-1826 .
        Hide
        Lari Hotari added a comment -

        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.

        Show
        Lari Hotari added a comment - 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.
        Hide
        Lari Hotari added a comment -

        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.

        Show
        Lari Hotari added a comment - 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.
        Hide
        Lari Hotari added a comment -

        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.

        Show
        Lari Hotari added a comment - 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.
        Hide
        Mark Smithson added a comment -

        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..

        Show
        Mark Smithson added a comment - 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..
        Hide
        Mark Smithson added a comment -

        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.

        Show
        Mark Smithson added a comment - 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.
        Hide
        Graeme Rocher added a comment -

        Moving to Groovy so the change can be done there

        Show
        Graeme Rocher added a comment - Moving to Groovy so the change can be done there
        Hide
        blackdrag blackdrag added a comment -

        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.

        Show
        blackdrag blackdrag added a comment - 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.
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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
        Hide
        Mark Smithson added a comment -

        Patch was Lari's, but yes was intended for Grails, not groovy.

        Show
        Mark Smithson added a comment - Patch was Lari's, but yes was intended for Grails, not groovy.
        Hide
        Lari Hotari added a comment -

        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).

        Show
        Lari Hotari added a comment - 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).
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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
        Hide
        blackdrag blackdrag added a comment -

        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.

        Show
        blackdrag blackdrag added a comment - 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.
        Hide
        Lari Hotari added a comment -

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

        Show
        Lari Hotari added a comment - This change might be causing problems in Grails 1.2-M4, see GRAILS-5345 .
        Hide
        Graeme Rocher added a comment -

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

        Show
        Graeme Rocher added a comment - Re-opening because of GRAILS-5345 , the consensus right now is to rollback the change for Groovy 1.6.6
        Hide
        blackdrag blackdrag added a comment -

        I reverted the change now

        Show
        blackdrag blackdrag added a comment - I reverted the change now
        Hide
        Graeme Rocher added a comment -
        Show
        Graeme Rocher added a comment - 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
        Hide
        blackdrag blackdrag added a comment -

        now really fixed

        Show
        blackdrag blackdrag added a comment - now really fixed
        Hide
        Graeme Rocher added a comment -

        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.

        Show
        Graeme Rocher added a comment - 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.
        Hide
        blackdrag blackdrag added a comment -

        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?

        Show
        blackdrag blackdrag added a comment - 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?
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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
        Hide
        Lari Hotari added a comment -

        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.

        Show
        Lari Hotari added a comment - 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.
        Hide
        Guillaume Laforge added a comment -

        I merged the pull request.

        Show
        Guillaume Laforge added a comment - I merged the pull request.
        Hide
        Serge P. Nekoval added a comment -

        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.

        Show
        Serge P. Nekoval added a comment - 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.
        Hide
        blackdrag blackdrag added a comment -

        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.

        Show
        blackdrag blackdrag added a comment - 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.
        Hide
        Lari Hotari added a comment -

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

        Show
        Lari Hotari added a comment - Serge, your stacktrace looks like GROOVY-5059 problem. It's fixed in Groovy 1.8.4 (and Grails 2.0).
        Hide
        Serge P. Nekoval added a comment -

        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...

        Show
        Serge P. Nekoval added a comment - 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...
        Hide
        Lari Hotari added a comment -

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

        Show
        Lari Hotari added a comment - Serge, agree. I should have said "improved in Groovy 1.8.4" instead of "fixed".

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Mark Smithson
          • Votes:
            2 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: