OpenJPA
  1. OpenJPA
  2. OPENJPA-250

Reduce synchronization bottlenecks in data cache, metadata repository, and lifecycle event manager

    Details

    • Patch Info:
      Patch Available

      Description

      Parts of the data cache, metadata repository, and lifecycle event manager are over-synchcronized. This should be resolved.

      1. sync-121.patch
        58 kB
        Simon Droscher
      2. OPENJPA-250-1.0.x.patch
        7 kB
        Jody Grassel
      3. OPENJPA-250.patch
        301 kB
        Patrick Linskey

        Activity

        Hide
        Patrick Linskey added a comment -

        This patch addresses the synchronization issues, and also replaces the OpenJPA-repackaged copies of Doug Lea's concurrent classes with a dependency on the backport-util-concurrent classes.

        Show
        Patrick Linskey added a comment - This patch addresses the synchronization issues, and also replaces the OpenJPA-repackaged copies of Doug Lea's concurrent classes with a dependency on the backport-util-concurrent classes.
        Hide
        Patrick Linskey added a comment -

        This patch addresses the synchronization issues, and also replaces the OpenJPA-repackaged copies of Doug Lea's concurrent classes with a dependency on the backport-util-concurrent classes.

        Show
        Patrick Linskey added a comment - This patch addresses the synchronization issues, and also replaces the OpenJPA-repackaged copies of Doug Lea's concurrent classes with a dependency on the backport-util-concurrent classes.
        Hide
        Patrick Linskey added a comment -

        Improvements have been made in this arena, but there is more to do.

        Show
        Patrick Linskey added a comment - Improvements have been made in this arena, but there is more to do.
        Hide
        Simon Droscher added a comment - - edited

        We found that caching performance hits a bottleneck with threads blocking on MetaDataRepository.getMetaData(). The changes in this issue looked to be the kind of thing that would help, but the original patch was for OpenJPA 0.9.x and a lot has changed in the code since then. Much of the patch was getting rid of classes in the org.apache.openjpa.lib.util.concurrent package and using edu.emory.mathcs.backport.java.util.concurrent classes instead. This looks to have been done at some other time in the code, using the standard java.util.concurrent package instead.

        So I went through all the code affected by the original patch and made similar changes as appropriate, basically changing a lot of synchronized methods to use blocks that lock and unlock using the ReentrantReadWriteLock() and ReentrantLock() classes.

        The resultant patch file is attached. This was found to give a much more scalable performance, we no longer have threads blocked on MetaDataRepository.getMetaData().

        Show
        Simon Droscher added a comment - - edited We found that caching performance hits a bottleneck with threads blocking on MetaDataRepository.getMetaData(). The changes in this issue looked to be the kind of thing that would help, but the original patch was for OpenJPA 0.9.x and a lot has changed in the code since then. Much of the patch was getting rid of classes in the org.apache.openjpa.lib.util.concurrent package and using edu.emory.mathcs.backport.java.util.concurrent classes instead. This looks to have been done at some other time in the code, using the standard java.util.concurrent package instead. So I went through all the code affected by the original patch and made similar changes as appropriate, basically changing a lot of synchronized methods to use blocks that lock and unlock using the ReentrantReadWriteLock() and ReentrantLock() classes. The resultant patch file is attached. This was found to give a much more scalable performance, we no longer have threads blocked on MetaDataRepository.getMetaData().
        Hide
        Donald Woods added a comment -

        Updating Affects, Fix and assignee values, given Curtis has checked this into trunk.

        Show
        Donald Woods added a comment - Updating Affects, Fix and assignee values, given Curtis has checked this into trunk.
        Hide
        Simon Droscher added a comment -

        Any chance of this making it into 1.2.2 or 1.3.0 ?

        Show
        Simon Droscher added a comment - Any chance of this making it into 1.2.2 or 1.3.0 ?
        Hide
        Michael Dick added a comment -

        Merging to 1.3.0 can be done, but I'm not ready to introduce it in 1.2.2 - synchronization changes make me nervous.

        Show
        Michael Dick added a comment - Merging to 1.3.0 can be done, but I'm not ready to introduce it in 1.2.2 - synchronization changes make me nervous.
        Hide
        Pinaki Poddar added a comment -

        1. Uses non-localized messages.
        2. preLoad and noLock are not documented

        Show
        Pinaki Poddar added a comment - 1. Uses non-localized messages. 2. preLoad and noLock are not documented
        Hide
        Rick Curtis added a comment -

        Shortly here I'm going to commit changes to this JIRA and I'd like to explain them before I pull the trigger...

        In doing some performance testing on trunk, we found that java.util.concurrent.lock.ReentrantLocks do not scale nearly as well as java synchronization. A 5% regression was observed on an 8-way and a ~20% regression was observed on a 16-way machine when using j.u.c.l.ReentrantLocks rather than the synchronized keyword. When openjpa.MetaDataRepository=Preload=true is configured this regression is gone, and a modest performance improvement is observed.

        The performance (or the lack thereof) of j.u.c.l.ReentrantLock is being pursued by the JDK team, but in the meantime I'm committing a change that will do the following:
        1.) Give us our previous 'default' performance.
        2.) Allow a user to configure the MDR to use no locks to improve scalability/performance.

        Show
        Rick Curtis added a comment - Shortly here I'm going to commit changes to this JIRA and I'd like to explain them before I pull the trigger... In doing some performance testing on trunk, we found that java.util.concurrent.lock.ReentrantLocks do not scale nearly as well as java synchronization. A 5% regression was observed on an 8-way and a ~20% regression was observed on a 16-way machine when using j.u.c.l.ReentrantLocks rather than the synchronized keyword. When openjpa.MetaDataRepository=Preload=true is configured this regression is gone, and a modest performance improvement is observed. The performance (or the lack thereof) of j.u.c.l.ReentrantLock is being pursued by the JDK team, but in the meantime I'm committing a change that will do the following: 1.) Give us our previous 'default' performance. 2.) Allow a user to configure the MDR to use no locks to improve scalability/performance.
        Hide
        Rick Curtis added a comment -

        Committed revision 898024 to trunk. This commit comes with the same comment that I noted for the 1.2 branch.

        Show
        Rick Curtis added a comment - Committed revision 898024 to trunk. This commit comes with the same comment that I noted for the 1.2 branch.
        Hide
        Rick Curtis added a comment -

        Sorry Mike, looks like I axed your last update!

        Show
        Rick Curtis added a comment - Sorry Mike, looks like I axed your last update!

          People

          • Assignee:
            Rick Curtis
            Reporter:
            Patrick Linskey
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development