Derby
  1. Derby
  2. DERBY-5623

Loosen up synchronization in FileMonitor

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.8.2.2
    • Fix Version/s: 10.9.1.0
    • Component/s: Services
    • Labels:
      None
    • Bug behavior facts:
      Performance

      Description

      When opening a large number of databases in parallel, many threads get stuck for a long time in FileMonitor, as reported in this thread on derby-user:
      http://mail-archives.apache.org/mod_mbox/db-derby-user/201202.mbox/%3CCAEQ8c00vx0yFrwWyjm%3Dt_Yd%2BuuKGOL%3DSgpYfQuYghOOPgdN8-w%40mail.gmail.com%3E

      The synchronization in FileMonitor is only needed because the monitor instance is also used as a shared PrivilegedExceptionAction instance. Since all databases share one FileMonitor instance, threads that access any of these methods are serialized. If each method created its own PrivilegedAction or PrivilegedExceptionAction instance instead of using the monitor's run() method, these methods wouldn't need synchronization.

      1. derby-diff.txt
        9 kB
        David Sitsky
      2. d5623-1.diff
        8 kB
        Knut Anders Hatlen

        Activity

        Knut Anders Hatlen created issue -
        Hide
        David Sitsky added a comment -

        Patch file to remove synchronisation in FileMonitor.

        Show
        David Sitsky added a comment - Patch file to remove synchronisation in FileMonitor.
        David Sitsky made changes -
        Field Original Value New Value
        Attachment derby-diff.txt [ 12515325 ]
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for the patch, David. I had played with a similar patch myself (see the attached file, d5623-1.diff). My patch differs a little from your patch:

        • It preserves the synchronization in getDaemonThread(). Without synchronization in that method, I saw occasional IllegalThreadStateExceptions when booting and shutting down databases concurrently in many threads.
        • It uses PrivilegedAction instead of PrivilegedExceptionAction for the actions that don't throw checked exceptions, which made the exception handling simpler.

        All the regression tests passed with this patch.

        I also ran an application that booted 1000 databases in 20 threads on a machine with 8 cores, and the patch seemed to speed it up by 3-4%. It was primarily the removal of synchronization in getJVMProperty() that contributed to the speed-up.

        Although the speed-up is modest (at least in my environment) the patches remove more lines of code than they add, which is a good thing in itself.

        Show
        Knut Anders Hatlen added a comment - Thanks for the patch, David. I had played with a similar patch myself (see the attached file, d5623-1.diff). My patch differs a little from your patch: It preserves the synchronization in getDaemonThread(). Without synchronization in that method, I saw occasional IllegalThreadStateExceptions when booting and shutting down databases concurrently in many threads. It uses PrivilegedAction instead of PrivilegedExceptionAction for the actions that don't throw checked exceptions, which made the exception handling simpler. All the regression tests passed with this patch. I also ran an application that booted 1000 databases in 20 threads on a machine with 8 cores, and the patch seemed to speed it up by 3-4%. It was primarily the removal of synchronization in getJVMProperty() that contributed to the speed-up. Although the speed-up is modest (at least in my environment) the patches remove more lines of code than they add, which is a good thing in itself.
        Knut Anders Hatlen made changes -
        Attachment d5623-1.diff [ 12515341 ]
        Hide
        David Sitsky added a comment -

        Great - sounds good.

        Show
        David Sitsky added a comment - Great - sounds good.
        Hide
        Knut Anders Hatlen added a comment -

        I committed the patch with revision 1292724. Marking the issue as resolved.

        Show
        Knut Anders Hatlen added a comment - I committed the patch with revision 1292724. Marking the issue as resolved.
        Knut Anders Hatlen made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Knut Anders Hatlen [ knutanders ]
        Fix Version/s 10.9.0.0 [ 12316344 ]
        Resolution Fixed [ 1 ]
        Knut Anders Hatlen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Workflow jira [ 12654025 ] Default workflow, editable Closed status [ 12796934 ]

          People

          • Assignee:
            Knut Anders Hatlen
            Reporter:
            Knut Anders Hatlen
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development