Velocity
  1. Velocity
  2. VELOCITY-595

ResourceManagerImpl.getResource() causes locking issues

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.6
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      jdk 1.5

      Description

      The ResourceManagerImpl.getResource() method is synchronized, which makes it difficult to share a Velocity Runtime between threads in an environment such as a j2ee web application.

      After upgrading Velocity to version 1.5 in Roller and running some performance tests I saw a very noticeable decrease in throughput for the application. I fired up jconsole and noticed that almost all of my app server threads were in a BLOCKED state and were waiting on the ResourceManagerImpl.getResource() method.

      In my particular case the difference resulted in a loss of 2/3 of my original ops/sec, which is pretty huge. After simply switching Velocity back to the 1.4 release and rerunning the test I saw the results I expected.

      I assume this is overactive use of Java synchronization because the developer guide suggests that the singleton model is "very appropriate model for use in a Servlet 2.2+ compliant web application".

      1. VELOCITY-595.jarkko.patch
        8 kB
        Nathan Bubna
      2. VELOCITY-595.patch
        2 kB
        Nathan Bubna

        Issue Links

          Activity

          Hide
          Nathan Bubna added a comment -

          Ok, i've run Velocity under heavy load with Jarkko's "pathetic little testbed" (VELOCITY-606), and i don't see any problems. I think this is safe and worthwhile.

          Show
          Nathan Bubna added a comment - Ok, i've run Velocity under heavy load with Jarkko's "pathetic little testbed" ( VELOCITY-606 ), and i don't see any problems. I think this is safe and worthwhile.
          Hide
          Nathan Bubna added a comment -

          Jarkko Viinamäki contributed an alternate patch for this issue as part of his performance improvement patch to VELOCITY-606. I've pulled this patch out, tidied it up and attached it here.

          Rather than synchronize resource fetching on a per-resource basis (as my patch does), he takes the "wasteful" approach of having both refreshResource and loadResource return new Resource instances for every request (updating the cache each time, as well). This keeps subsequent requests from stomping on each other's Template instances (as was happening in VELOCITY-24 via the resource.process() call in refreshResource) by letting simultaneous requests always have their own separate Template instance.

          Again, i still don't have access to the testbed app Henning was using to test these things (to my knowledge), so i can't personally confirm that this doesn't re-introduce VELOCITY-24, but the fix makes sense to me. Also, Jarkko is doing multi-thread load testing as part of VELOCITY-606 and has not hit the bug with this patch in place. So, i think this is sound. I'll leave some time for comment and for Jarkko to retest his VELOCITY-606 changes (including this) once they're adapted to JDK 1.4 before i commit this.

          Show
          Nathan Bubna added a comment - Jarkko Viinamäki contributed an alternate patch for this issue as part of his performance improvement patch to VELOCITY-606 . I've pulled this patch out, tidied it up and attached it here. Rather than synchronize resource fetching on a per-resource basis (as my patch does), he takes the "wasteful" approach of having both refreshResource and loadResource return new Resource instances for every request (updating the cache each time, as well). This keeps subsequent requests from stomping on each other's Template instances (as was happening in VELOCITY-24 via the resource.process() call in refreshResource) by letting simultaneous requests always have their own separate Template instance. Again, i still don't have access to the testbed app Henning was using to test these things (to my knowledge), so i can't personally confirm that this doesn't re-introduce VELOCITY-24 , but the fix makes sense to me. Also, Jarkko is doing multi-thread load testing as part of VELOCITY-606 and has not hit the bug with this patch in place. So, i think this is sound. I'll leave some time for comment and for Jarkko to retest his VELOCITY-606 changes (including this) once they're adapted to JDK 1.4 before i commit this.
          Hide
          Nathan Bubna added a comment -

          ping Henning, Allen Gililand, Ilkka Priha

          anyone out there? the patch i submitted passes all Velocity tests in SVN, but as there is no test there for the issue at hand i could really use some feedback here.

          Show
          Nathan Bubna added a comment - ping Henning, Allen Gililand, Ilkka Priha anyone out there? the patch i submitted passes all Velocity tests in SVN, but as there is no test there for the issue at hand i could really use some feedback here.
          Hide
          Nathan Bubna added a comment -

          Ok, i read through VELOCITY-24 more carefully. It looks like this was originally an issue with inline macros when caching is on and auto-reloading is on. But then Henning proffers a fix that hurts those who do not use caching, with no mention of auto-reloading or not. It's clear that the fix worked for the original problem, but without more investigation and/or input from Henning, i'm not sure if we can limit the synchronization at all. At the least, it seems that loadResource() is not the issue, but rather refreshResource() and perhaps the put/get of a Resource into the cache and the return of the Resource.

          Allen, what cache/reload settings are you using when you experience the slowdown?

          Henning, are you following this? Can we get the test template and/or help reproducing this?

          I know it is difficult to properly synchronize, but it seems like we could carefully use the resourceKey created at the start of getResource() as a lock object to be more fine-grained about this. The patch i attached is totally untested (didn't even compile it) and really just a rough idea, but i suspect something like it would solve the race condition for VELOCITY-24 with less thread contention.

          Show
          Nathan Bubna added a comment - Ok, i read through VELOCITY-24 more carefully. It looks like this was originally an issue with inline macros when caching is on and auto-reloading is on. But then Henning proffers a fix that hurts those who do not use caching, with no mention of auto-reloading or not. It's clear that the fix worked for the original problem, but without more investigation and/or input from Henning, i'm not sure if we can limit the synchronization at all. At the least, it seems that loadResource() is not the issue, but rather refreshResource() and perhaps the put/get of a Resource into the cache and the return of the Resource. Allen, what cache/reload settings are you using when you experience the slowdown? Henning, are you following this? Can we get the test template and/or help reproducing this? I know it is difficult to properly synchronize, but it seems like we could carefully use the resourceKey created at the start of getResource() as a lock object to be more fine-grained about this. The patch i attached is totally untested (didn't even compile it) and really just a rough idea, but i suspect something like it would solve the race condition for VELOCITY-24 with less thread contention.
          Hide
          Ilkka Priha added a comment -

          Wouldn't it be enough to synchronize the loadResource() method only, now reading from the cache is also synchronized. The benchmark comparing the effects of this change was run with a duo-core processor, but in heavy duty servers we may have 8-16 processors where synchronization slows down performance much more than in simpler cases (couldn't find the test template to reproduce and test VELOCITY-24).

          Show
          Ilkka Priha added a comment - Wouldn't it be enough to synchronize the loadResource() method only, now reading from the cache is also synchronized. The benchmark comparing the effects of this change was run with a duo-core processor, but in heavy duty servers we may have 8-16 processors where synchronization slows down performance much more than in simpler cases (couldn't find the test template to reproduce and test VELOCITY-24 ).
          Hide
          Nathan Bubna added a comment -

          This is not "overactive use" as it fixed a known problem. Unfortunately, speed in an application that is not caching resources must come second to correctness.

          Show
          Nathan Bubna added a comment - This is not "overactive use" as it fixed a known problem. Unfortunately, speed in an application that is not caching resources must come second to correctness.
          Hide
          Nathan Bubna added a comment -

          This is a known side effect of the fix for VELOCITY-24.
          http://svn.apache.org/viewvc?view=rev&revision=471246

          Show
          Nathan Bubna added a comment - This is a known side effect of the fix for VELOCITY-24 . http://svn.apache.org/viewvc?view=rev&revision=471246

            People

            • Assignee:
              Unassigned
              Reporter:
              Allen Gilliland
            • Votes:
              1 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development