Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.4
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Similar issues:
      KULRICE-1341Some documents that spawn other documents don't route to final when workflow run in synchronous mode.
      KULRICE-7635GlobalResourceLoader is over-synchronizing on GRL.getService
      KULRICE-8894"Synchronous" behavior w/in KRAD
      KULRICE-9803Remove interpret freemarker calls
      KULRICE-9477Freemarker error with column calculation and submit of maintenance document
      KULRICE-10548Freemarker rendering phase extension
      KULRICE-8442Module locking needs converted to FreeMarker
      KULRICE-8710Template Browser Mob tests with freemarker
      KULRICE-1857Synchronize DTOs with BOs
      KULRICE-13780Create Freemarker 404 AFT
    • Epic Link:
    • Rice Module:
      KRAD
    • KRAD Feature Area:
      UIF MVC
    • Sprint:
      2.4.0-m2 Sprint 2
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Include in Release Notes?:
      Yes

      Description

      freemarker.ext.beans.BeansWrapper has a couple of heavily used methods that perform their workload within a synchronize block. This causes a krad application to perform more like it is a single threaded application.

      I've opened an issue with the freemarker team as well with the ID: 3604140.

      I'll attach a version of the BeansWrapper class that can be used with version 2.3.19 (this required rice 2.2.0 to change from version 2.3.18)

      1. BeansWrapper.java
        69 kB
        Jeff Domeyer
      2. kulrice-8949-fm2.3.19-patch4.patch
        13 kB
        Mark Fyffe

        Issue Links

          Activity

          Hide
          Jeff Domeyer (Inactive) added a comment -

          There's a missed System.out.println I left in that file, so remove that before actually using

          Show
          Jeff Domeyer (Inactive) added a comment - There's a missed System.out.println I left in that file, so remove that before actually using
          Hide
          Mark Fyffe (Inactive) added a comment -

          Below is a diff illustrating the differences in Jeff's original contribution.

          --- BeansWrapper.java   2013-08-19 06:57:10.282267400 -0400
          +++ /cygdrive/c/Users/mark/Downloads/BeansWrapper.java  2013-08-19 06:10:20.423960200 -0400
          @@ -962,11 +962,15 @@
               
               void introspectClass(Class clazz)
               {
          -        synchronized(classCache)
          +        if(!classCache.containsKey(clazz))
                   {
          -            if(!classCache.containsKey(clazz))
          +            System.out.println(clazz);
          +            synchronized(classCache)
                       {
          -                introspectClassInternal(clazz);
          +                if(!classCache.containsKey(clazz))
          +                {
          +                    introspectClassInternal(clazz);
          +                }
                       }
                   }
               }
          @@ -1013,14 +1017,16 @@
           
               Map getClassKeyMap(Class clazz)
               {
          -        Map map;
          -        synchronized(classCache)
          -        {
          -            map = (Map)classCache.get(clazz);
          -            if(map == null)
          +        Map map = (Map)classCache.get(clazz);
          +        if(map == null){
          +            synchronized(classCache)
                       {
          -                introspectClassInternal(clazz);
                           map = (Map)classCache.get(clazz);
          +                if(map == null)
          +                {
          +                    introspectClassInternal(clazz);
          +                    map = (Map)classCache.get(clazz);
          +                }
                       }
                   }
                   return map;
          

          These changes appear not to be entirely safe, since the backing classCache is a non-synchronized HashMap and therefore not safe for multithreaded access, even if that access is read-only. The double-initialization blocking is still in place in this version, but cache references are no longer thread-safe.

          Through some quick analysis of BeansWrapper and related files ModelCache, BeansModelCache, and BeanModel I'm noting that the use of unsynchronized HashMap is pervasive throughout FreeMarker, and that lazy-initialing cache references are all blocking - this is apparently for the purpose of preventing double-initialization of cached wrappers, but also appears to be unnecessary for the wrappers in question.

          I'm also noting that none of the class-keyed caches use weak keys, so if FreeMarker is used with beans loaded from a different classloader, it could very easily prevent that class's loader from unloading.

          From the current state of kuali-freemarker, I would suggest a few changes to reduce blocking.

          1. Where Class keys are used, replace HashMap with Collections.sycnhronizedMap(WeakHashMap)
          2. Remove synchronization blocks intended to prevent double-initialization of cached values.
          3. Remove synchronization blocks surrounding reference queue polling.

          However, this is FreeMarker (not Rice), so these changes would appear to be too invasive At the very least, they are too invasive without first syncing up with the FreeMarker development state.

          According to http://sourceforge.net/p/freemarker/bugs/382/, the locking issues have been fixed in 2.3.20, and upon close inspection of the current FreeMarker source, BeansWrapper is sufficiently different. For example, the getClassKeyMap() no longer exists. There have been roughly 14 commits in the FreeMarker 2.3 branch since they migrated to GitHub, and this was at least 1 month after the team reported the bug as fixed.

          It would seem prudent, therefore, to start by synching up kuali-freemarker with the state of FreeMarker 2.3 development and testing from there. It is possible that we might pick up a few other performance gains this way as well

          I'll look further into upgrading FreeMarker next weekend.

          Show
          Mark Fyffe (Inactive) added a comment - Below is a diff illustrating the differences in Jeff's original contribution. --- BeansWrapper.java 2013-08-19 06:57:10.282267400 -0400 +++ /cygdrive/c/Users/mark/Downloads/BeansWrapper.java 2013-08-19 06:10:20.423960200 -0400 @@ -962,11 +962,15 @@ void introspectClass(Class clazz) { - synchronized(classCache) + if(!classCache.containsKey(clazz)) { - if(!classCache.containsKey(clazz)) + System.out.println(clazz); + synchronized(classCache) { - introspectClassInternal(clazz); + if(!classCache.containsKey(clazz)) + { + introspectClassInternal(clazz); + } } } } @@ -1013,14 +1017,16 @@ Map getClassKeyMap(Class clazz) { - Map map; - synchronized(classCache) - { - map = (Map)classCache.get(clazz); - if(map == null) + Map map = (Map)classCache.get(clazz); + if(map == null){ + synchronized(classCache) { - introspectClassInternal(clazz); map = (Map)classCache.get(clazz); + if(map == null) + { + introspectClassInternal(clazz); + map = (Map)classCache.get(clazz); + } } } return map; These changes appear not to be entirely safe, since the backing classCache is a non-synchronized HashMap and therefore not safe for multithreaded access, even if that access is read-only. The double-initialization blocking is still in place in this version, but cache references are no longer thread-safe. Through some quick analysis of BeansWrapper and related files ModelCache, BeansModelCache, and BeanModel I'm noting that the use of unsynchronized HashMap is pervasive throughout FreeMarker, and that lazy-initialing cache references are all blocking - this is apparently for the purpose of preventing double-initialization of cached wrappers, but also appears to be unnecessary for the wrappers in question. I'm also noting that none of the class-keyed caches use weak keys, so if FreeMarker is used with beans loaded from a different classloader, it could very easily prevent that class's loader from unloading. From the current state of kuali-freemarker, I would suggest a few changes to reduce blocking. Where Class keys are used, replace HashMap with Collections.sycnhronizedMap(WeakHashMap) Remove synchronization blocks intended to prevent double-initialization of cached values. Remove synchronization blocks surrounding reference queue polling. However, this is FreeMarker (not Rice), so these changes would appear to be too invasive At the very least, they are too invasive without first syncing up with the FreeMarker development state. According to http://sourceforge.net/p/freemarker/bugs/382/ , the locking issues have been fixed in 2.3.20, and upon close inspection of the current FreeMarker source, BeansWrapper is sufficiently different. For example, the getClassKeyMap() no longer exists. There have been roughly 14 commits in the FreeMarker 2.3 branch since they migrated to GitHub, and this was at least 1 month after the team reported the bug as fixed. It would seem prudent, therefore, to start by synching up kuali-freemarker with the state of FreeMarker 2.3 development and testing from there. It is possible that we might pick up a few other performance gains this way as well I'll look further into upgrading FreeMarker next weekend.
          Hide
          Mark Fyffe (Inactive) added a comment -

          Attached patch encapsulating differences between freemarker-2.3.19 and 2.3.19-patch4.

          Show
          Mark Fyffe (Inactive) added a comment - Attached patch encapsulating differences between freemarker-2.3.19 and 2.3.19-patch4.
          Hide
          Mark Fyffe (Inactive) added a comment -

          Performed upgrade locally by replacing kuali-freemaker contents with https://github.com/freemarker/freemarker/branches/2.3

          Updated IML to refer to new project structure, but haven't tested with IntelliJ.

          Eclipse development works well using IvyDE - Eclipse software install URL: http://www.apache.org/dist/ant/ivyde/updatesite

          Reapplied DynamicCall.java, and references in FTL.jj from 2.3.19 patch.

          Added missing methods to DynamicCall for compliance with 2.3.20+

          Manually installed to local M2 repos as org.freemarker:freemarker:2.3.20-patch1

          Modified freemarker version in Rice parent POM, and reinstalled locally.

          Tested and verified correct operation of KRAD sampleapp.

          Coordinating with Jerry on the correct process to add this update for access from krad-web-framework - will commit once this process has been confirmed.

          Show
          Mark Fyffe (Inactive) added a comment - Performed upgrade locally by replacing kuali-freemaker contents with https://github.com/freemarker/freemarker/branches/2.3 Updated IML to refer to new project structure, but haven't tested with IntelliJ. Eclipse development works well using IvyDE - Eclipse software install URL: http://www.apache.org/dist/ant/ivyde/updatesite Reapplied DynamicCall.java, and references in FTL.jj from 2.3.19 patch. Added missing methods to DynamicCall for compliance with 2.3.20+ Manually installed to local M2 repos as org.freemarker:freemarker:2.3.20-patch1 Modified freemarker version in Rice parent POM, and reinstalled locally. Tested and verified correct operation of KRAD sampleapp. Coordinating with Jerry on the correct process to add this update for access from krad-web-framework - will commit once this process has been confirmed.
          Hide
          Mark Fyffe (Inactive) added a comment -

          FWIW - my patch numbers appear to be a little off today. References to "2.3.19-patch4" should have been "2.3.19-patch3"

          Show
          Mark Fyffe (Inactive) added a comment - FWIW - my patch numbers appear to be a little off today. References to "2.3.19-patch4" should have been "2.3.19-patch3"
          Hide
          Mark Fyffe (Inactive) added a comment -

          Committed 2.3.20 upgrade to kuali-freemarker

          Show
          Mark Fyffe (Inactive) added a comment - Committed 2.3.20 upgrade to kuali-freemarker
          Hide
          Mark Fyffe (Inactive) added a comment -

          Upgrade to 2.3.20 complete. According to FreeMarker issue tracking, synchronization on BeansWrapper has been reduced in this version.

          Further work to reduce BeansWrapper overhead is in KULRICE-10353.

          Per Jerry Neal, freemarker-2.3.20-patch1 has been added to Kuali Nexus. This version includes the #dyncall custom directive.

          Show
          Mark Fyffe (Inactive) added a comment - Upgrade to 2.3.20 complete. According to FreeMarker issue tracking, synchronization on BeansWrapper has been reduced in this version. Further work to reduce BeansWrapper overhead is in KULRICE-10353 . Per Jerry Neal, freemarker-2.3.20-patch1 has been added to Kuali Nexus. This version includes the #dyncall custom directive.
          Hide
          Mark Fyffe (Inactive) added a comment -

          Switched 2.3 branch to freemarker-2.3.20-patch1

          Show
          Mark Fyffe (Inactive) added a comment - Switched 2.3 branch to freemarker-2.3.20-patch1

            People

            • Assignee:
              Mark Fyffe (Inactive)
              Reporter:
              Jeff Domeyer (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 6 hours Original Estimate - 6 hours
                6h
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 7 hours
                7h

                  Agile

                    Structure Helper Panel