Tapestry 5
  1. Tapestry 5
  2. TAP5-945

Unnecessary and severe lock contention in PerthreadManagerImpl

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 5.1.0.5
    • Fix Version/s: 5.2.0
    • Component/s: tapestry-core
    • Labels:
      None

      Description

      When load testing our new high-volume site before soft launch, we found that we have severe lock contention in org.apache.tapestry5.ioc.internal.services.PerthreadManagerImpl.

      It synchronizes on "this" before invoking ThreadLocal.get() and ThreadLocal.remove(), which I believe is unnecessary.

      During our tests, approximately 35% of all Tomcat threads were waiting for this lock in any of 10 thread dumps taken 15 seconds apart.

        Issue Links

          Activity

          Hide
          Olle Hallin added a comment -

          We tested with 1000 virtual users and 300 Tomcat threads.

          Show
          Olle Hallin added a comment - We tested with 1000 virtual users and 300 Tomcat threads.
          Hide
          Howard M. Lewis Ship added a comment -

          As I mentioned on the mailing list, this addresses a bug that may still be present in Sun JDK 1.5, that ThreadLocal is not fully thread safe in some situations ... the kind of situations that Tapestry hits.

          This could be recoded in a couple of ways. We could use a configuration symbol or JVM system property to enable/disable the synchronization. We could add a test for JDK 1.6 to disable the synchronization. We could use a ReentrantReadWriteLock to allow shared readers (the common case).

          Show
          Howard M. Lewis Ship added a comment - As I mentioned on the mailing list, this addresses a bug that may still be present in Sun JDK 1.5, that ThreadLocal is not fully thread safe in some situations ... the kind of situations that Tapestry hits. This could be recoded in a couple of ways. We could use a configuration symbol or JVM system property to enable/disable the synchronization. We could add a test for JDK 1.6 to disable the synchronization. We could use a ReentrantReadWriteLock to allow shared readers (the common case).
          Hide
          Massimo Lusetti added a comment -

          I will definitely for the latter.

          BTW Olle could you try to remove the sync blocks and see what happen?

          Show
          Massimo Lusetti added a comment - I will definitely for the latter. BTW Olle could you try to remove the sync blocks and see what happen?
          Hide
          Howard M. Lewis Ship added a comment -

          The JDK bug was fixed in JDK6 (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5025230) but may still exist in JDK 1.5.

          Best option would be to disable the synchronization unless JDK 1.5. That would be nice ... some free performance boost for using JDK 1.6.

          ReentrantReadWriteLock would also work but there's a bit of overhead there.

          Show
          Howard M. Lewis Ship added a comment - The JDK bug was fixed in JDK6 ( http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5025230 ) but may still exist in JDK 1.5. Best option would be to disable the synchronization unless JDK 1.5. That would be nice ... some free performance boost for using JDK 1.6. ReentrantReadWriteLock would also work but there's a bit of overhead there.
          Hide
          Howard M. Lewis Ship added a comment -

          The synchronization logic is only present for JDK 1.5 now; for JDK 1.6 or above, no extra synchronization occurs.

          Show
          Howard M. Lewis Ship added a comment - The synchronization logic is only present for JDK 1.5 now; for JDK 1.6 or above, no extra synchronization occurs.
          Hide
          Joost Schouten added a comment -

          I'm not quite sure if it is possible to run Tapestry5 on java 1.4 and if anyone does, but looking at the current code the DummyLock will be used with anything but java 1.5. Is that what you want? Should it not be; use DummyLock when java version > 1.5?

          Show
          Joost Schouten added a comment - I'm not quite sure if it is possible to run Tapestry5 on java 1.4 and if anyone does, but looking at the current code the DummyLock will be used with anything but java 1.5. Is that what you want? Should it not be; use DummyLock when java version > 1.5?
          Hide
          Olle Hallin added a comment -

          T5 requires Java 1.5+

          Show
          Olle Hallin added a comment - T5 requires Java 1.5+
          Hide
          Joost Schouten added a comment -

          that'll fix it then thx. Just wondered since the JDK bug was reported against 1.4.

          Show
          Joost Schouten added a comment - that'll fix it then thx. Just wondered since the JDK bug was reported against 1.4.

            People

            • Assignee:
              Howard M. Lewis Ship
              Reporter:
              Olle Hallin
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development