Struts 2
  1. Struts 2
  2. WW-3750

ScopesHashModel calls OgnlValueStack.findValue too many times during rendering freemarker templates

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1.1
    • Fix Version/s: 2.3.3
    • Component/s: Value Stack
    • Labels:
    • Flags:
      Patch

      Description

      I saw using a profiler, that OgnlValueStack.findValue(String) gets called about a thousand times during rendering a page. Most of the calls are coming from ScopesHashModel.
      All freemarker templates contain a lot of references to e.g. "parameters". This variable is evaluated in ScopesHashModel over and over again, which takes about 10% time of total page load.
      I think we can assume, that the top-level objects on the value stack will not change during rendering a single struts2 tag. So with a little caching, we can eliminate most of the findValue method calls.

      In my application I tested this modification and didn't notice any side effects or bugs. The OgnlValueStack.findValue(String) calls on a test page went down from a thousand to a hundred. This improved overall page rendering time from about 400ms to 360ms.

      Patch is attached.
      Please review it.

      1. WW-3750.diff
        1 kB
        Pelladi Gabor
      2. WW-3750-2.diff
        2 kB
        Pelladi Gabor
      3. WW-3750-parameters.diff
        2 kB
        Pelladi Gabor

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Struts2 #439 (See https://builds.apache.org/job/Struts2/439/)
          WW-3750 reduces scope of cashing (Revision 1307614)

          Result = SUCCESS
          lukaszlenart :
          Files :

          • /struts/struts2/trunk/core/src/main/java/org/apache/struts2/views/freemarker/ScopesHashModel.java
          • /struts/struts2/trunk/plugins/config-browser/src/main/resources/config-browser/actionNames.ftl
          • /struts/struts2/trunk/plugins/config-browser/src/main/resources/config-browser/page-header.ftl
          Show
          Hudson added a comment - Integrated in Struts2 #439 (See https://builds.apache.org/job/Struts2/439/ ) WW-3750 reduces scope of cashing (Revision 1307614) Result = SUCCESS lukaszlenart : Files : /struts/struts2/trunk/core/src/main/java/org/apache/struts2/views/freemarker/ScopesHashModel.java /struts/struts2/trunk/plugins/config-browser/src/main/resources/config-browser/actionNames.ftl /struts/struts2/trunk/plugins/config-browser/src/main/resources/config-browser/page-header.ftl
          Hide
          Lukasz Lenart added a comment -

          Done, thanks for the patch!

          Show
          Lukasz Lenart added a comment - Done, thanks for the patch!
          Hide
          Pelladi Gabor added a comment -

          Patch that only caches the parameters object.

          Show
          Pelladi Gabor added a comment - Patch that only caches the parameters object.
          Hide
          Pelladi Gabor added a comment - - edited

          I see.
          I wrote in the description: "I think we can assume, that the top-level objects on the value stack will not change during rendering a single struts2 tag."
          It seems that this assumption is false. This code:

          <@s.url id="showConfig" action="showConfig" includeParams="none">
          

          Modifies the top-level object on the value stack named "showConfig". But with the caching in place, it is evaluated only once. If this is within a loop, this can cause regression.
          By using the struts2 "a" tag, the variable will not be evaluated as a freemarker variable, but directly as a value stack variable. It does not use ScopesHashModel. That is why it works.

          We have the following options:

          • Cache only the parameters object. Every other variable will not be cached. This keeps the majority of the performance improvement without affecting compatibility.
          • Keep the current code, and use <@s.a> instead of <a> as you mentioned. Generally, all occurrences of <@s.url> inside a loop should be reviewed.

          For the first option, I am attaching a patch.
          The parametersCache field in the patch is made thread-safe using the volatile keyword, which is enough for this usage.

          Show
          Pelladi Gabor added a comment - - edited I see. I wrote in the description: "I think we can assume, that the top-level objects on the value stack will not change during rendering a single struts2 tag." It seems that this assumption is false. This code: <@s.url id= "showConfig" action= "showConfig" includeParams= "none" > Modifies the top-level object on the value stack named "showConfig". But with the caching in place, it is evaluated only once. If this is within a loop, this can cause regression. By using the struts2 "a" tag, the variable will not be evaluated as a freemarker variable, but directly as a value stack variable. It does not use ScopesHashModel. That is why it works. We have the following options: Cache only the parameters object. Every other variable will not be cached. This keeps the majority of the performance improvement without affecting compatibility. Keep the current code, and use <@s.a> instead of <a> as you mentioned. Generally, all occurrences of <@s.url> inside a loop should be reviewed. For the first option, I am attaching a patch. The parametersCache field in the patch is made thread-safe using the volatile keyword, which is enough for this usage.
          Hide
          Lukasz Lenart added a comment -

          With this patch in place the above example is invalid, to be valid instead using <a/> tag, <@s.a href="%

          {showConfig}

          >$

          {name}

          </@s.a> must be used. Any thoughts ?

          Show
          Lukasz Lenart added a comment - With this patch in place the above example is invalid, to be valid instead using <a/> tag, <@s.a href="% {showConfig} >$ {name} </@s.a> must be used. Any thoughts ?
          Hide
          Lukasz Lenart added a comment - - edited

          This patch has a side effect on how <#list> (<#foreach> and so on) are working in Freemarker templates.

          Take a look on the exercise below from the config-browser plugin. With patch in place, $

          {showConfig} expression will be evaluated only once throughout the Stack and the returned value will always be the same.
          
          

          <#list actionNames as name>
          <@s.url id="showConfig" action="showConfig" includeParams="none">
          <@s.param name="namespace">${namespace}</@s.param>
          <@s.param name="actionName">${name}</@s.param>
          </@s.url>
          <li><a href="${showConfig}

          ">$

          {name}

          </a></li>
          </#list>

          
          
          Show
          Lukasz Lenart added a comment - - edited This patch has a side effect on how <#list> (<#foreach> and so on) are working in Freemarker templates. Take a look on the exercise below from the config-browser plugin. With patch in place, $ {showConfig} expression will be evaluated only once throughout the Stack and the returned value will always be the same. <#list actionNames as name> <@s.url id="showConfig" action="showConfig" includeParams="none"> <@s.param name="namespace">${namespace}</@s.param> <@s.param name="actionName">${name}</@s.param> </@s.url> <li><a href="${showConfig} ">$ {name} </a></li> </#list>
          Hide
          Hudson added a comment -

          Integrated in Struts2 #412 (See https://builds.apache.org/job/Struts2/412/)
          WW-3750: ScopesHashModel calls OgnlValueStack.findValue too many times during rendering freemarker templates
          Patch provided by Pelladi Gabor

          jogep :
          Files :

          • /struts/struts2/trunk/core/src/main/java/org/apache/struts2/views/freemarker/ScopesHashModel.java
          Show
          Hudson added a comment - Integrated in Struts2 #412 (See https://builds.apache.org/job/Struts2/412/ ) WW-3750 : ScopesHashModel calls OgnlValueStack.findValue too many times during rendering freemarker templates Patch provided by Pelladi Gabor jogep : Files : /struts/struts2/trunk/core/src/main/java/org/apache/struts2/views/freemarker/ScopesHashModel.java
          Hide
          Johannes Geppert added a comment -

          I've applied the Patch. Thank you for providing this Patch.

          Show
          Johannes Geppert added a comment - I've applied the Patch. Thank you for providing this Patch.
          Hide
          Johannes Geppert added a comment -

          The Patch looks good for me. If no one has an recall I will going to include it into next release.

          Show
          Johannes Geppert added a comment - The Patch looks good for me. If no one has an recall I will going to include it into next release.
          Hide
          Pelladi Gabor added a comment -

          Thread-safe version of patch

          Show
          Pelladi Gabor added a comment - Thread-safe version of patch
          Hide
          Pelladi Gabor added a comment -

          Indeed, an instance of ScopesHashModel is not thread-safe. But as I looked through the source I have concluded that it doesn't needs to be thread-safe.

          Here is what I did. I opened the call hierarchy for the constructor of ScopesHashModel. This gets called only from FreemarkerManager.buildScopesHashModel(), and that from FreemarkerManager.buildTemplateModel(). From here there are two paths:
          1. FreemarkerResult.createModel(), then FreemarkerResult.doExecute(). Here the ScopesHashModel is assigned to a local variable, and after exiting the method, the ScopesHashModel will be garbage collected.
          2. FreemarkerTemplateEngine.renderTemplate(). Here the situation is similar, the ScopesHashModel is a local variable and will be garbage collected.

          So it seems that an instance of ScopesHashModel is not used by multiple threads. For every struts2 tag rendering, a new instance is created. An other field of this class, unlistedModels is also a simple HashMap, so the class is not totally thread-safe even in it's current form.

          But it is true that there may be a plugin or any code using this class in a way that multiple threads operate on the same instance. The base class, freemarker.template.SimpleHash is a bit more thread safe, javadoc says: "This class is thread-safe if you don't call the put or remove methods after you have made the object available for multiple threads.".

          So I have created another patch that uses a ConcurrentHashMap. For this the code had to be modified also, because ConcurrentHashMap (unlike HashMap) does not handle null values.

          Show
          Pelladi Gabor added a comment - Indeed, an instance of ScopesHashModel is not thread-safe. But as I looked through the source I have concluded that it doesn't needs to be thread-safe. Here is what I did. I opened the call hierarchy for the constructor of ScopesHashModel. This gets called only from FreemarkerManager.buildScopesHashModel(), and that from FreemarkerManager.buildTemplateModel(). From here there are two paths: 1. FreemarkerResult.createModel(), then FreemarkerResult.doExecute(). Here the ScopesHashModel is assigned to a local variable, and after exiting the method, the ScopesHashModel will be garbage collected. 2. FreemarkerTemplateEngine.renderTemplate(). Here the situation is similar, the ScopesHashModel is a local variable and will be garbage collected. So it seems that an instance of ScopesHashModel is not used by multiple threads. For every struts2 tag rendering, a new instance is created. An other field of this class, unlistedModels is also a simple HashMap, so the class is not totally thread-safe even in it's current form. But it is true that there may be a plugin or any code using this class in a way that multiple threads operate on the same instance. The base class, freemarker.template.SimpleHash is a bit more thread safe, javadoc says: "This class is thread-safe if you don't call the put or remove methods after you have made the object available for multiple threads.". So I have created another patch that uses a ConcurrentHashMap. For this the code had to be modified also, because ConcurrentHashMap (unlike HashMap) does not handle null values.
          Hide
          Lukasz Lenart added a comment -

          Is it thread-safe ? The patch uses simple HashMap, but maybe I missed something.

          Show
          Lukasz Lenart added a comment - Is it thread-safe ? The patch uses simple HashMap, but maybe I missed something.
          Hide
          Pelladi Gabor added a comment -

          Patch that improves performance

          Show
          Pelladi Gabor added a comment - Patch that improves performance

            People

            • Assignee:
              Johannes Geppert
              Reporter:
              Pelladi Gabor
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development