[ 
https://issues.apache.org/jira/browse/WW-3750?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13233377#comment-13233377
 ] 

Pelladi Gabor edited comment on WW-3750 at 3/20/12 12:43 PM:
-------------------------------------------------------------

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:
{code}
<@s.url id="showConfig" action="showConfig" includeParams="none">
{code}
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.
                
      was (Author: pelladi):
    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:
{code}
<@s.url id="showConfig" action="showConfig" includeParams="none">
{code}
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.
                  
> ScopesHashModel calls OgnlValueStack.findValue too many times during 
> rendering freemarker templates
> ---------------------------------------------------------------------------------------------------
>
>                 Key: WW-3750
>                 URL: https://issues.apache.org/jira/browse/WW-3750
>             Project: Struts 2
>          Issue Type: Improvement
>          Components: Value Stack
>    Affects Versions: 2.3.1.1
>            Reporter: Pelladi Gabor
>            Assignee: Johannes Geppert
>            Priority: Minor
>              Labels: patch, performance
>             Fix For: 2.3.2
>
>         Attachments: WW-3750-2.diff, WW-3750-parameters.diff, WW-3750.diff
>
>
> 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.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to