[jira] [Commented] (WW-5147) OGNL valid expression is not cached and is parsed over again in some situations

["Lukasz Lenart (Jira)" <ji...@apache.org>]

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

Lukasz Lenart commented on WW-5147:
-----------------------------------

Thanks a lot [~davoustp] for you investigation and report. Could you create an issue here [https://github.com/jkuhnert/ognl|https://github.com/jkuhnert/ognl?] and open a PR? I will be more than happy to check this and merge :)

> OGNL valid expression is not cached and is parsed over again in some situations
> -------------------------------------------------------------------------------
>
>                 Key: WW-5147
>                 URL: https://issues.apache.org/jira/browse/WW-5147
>             Project: Struts 2
>          Issue Type: Bug
>          Components: Core
>    Affects Versions: 2.5.20
>            Reporter: Pascal Davoust
>            Priority: Major
>
> Profiling an enterprise application under high load shows an unlikely number of invocations to method {{ognl.Ognl.parseExpression}} (representing a significant cumulated CPU time), despite having the OGNL expression cache enabled.
> Knowing that there is no dynamic expression in this application, this is puzzling: once each expression have been encountered at least once, its parsed/compiled representation should be cached, avoiding the parsing phase entirely to only keep the execution phase.
> After investigation, this is due to the caching logic in method {{com.opensymphony.xwork2.ognl.OgnlUtil.compileAndExecute(String, Map<String, Object>, OgnlTask<T>}} (same applies to {{com.opensymphony.xwork2.ognl.OgnlUtil.compileAndExecuteMethod(String, Map<String, Object>, OgnlTask<T>)}} ) :
>  
> {code:java}
> private <T> Object compileAndExecute(String expression, Map<String, Object> context, OgnlTask<T> task) throws OgnlException {
>     Object tree;
>     if (enableExpressionCache) {
>         tree = expressions.get(expression);
>         if (tree == null) {
>             tree = Ognl.parseExpression(expression);
>             checkEnableEvalExpression(tree, context);
>         }
>     } else {
>         tree = Ognl.parseExpression(expression);
>         checkEnableEvalExpression(tree, context);
>     }
>     final T exec = task.execute(tree);
>     // if cache is enabled and it's a valid expression, puts it in
>     if (enableExpressionCache) {
>         expressions.putIfAbsent(expression, tree);
>     }
>     return exec;
> } {code}
>  
>  
> As shown above, on cache miss, the expression is parsed, then executed, and added to the cache {+}only after execution{+}.
> Problem is that the execution can fail (especially that Ognl makes use of exceptions quite extensively): in this case, the parsed AST is lost and not added to the cache.
> As a result, the same expression will go through the cache miss code path next time.
>  
> Unless I'm mistaken, the AST of the parsed expression could be added to the cache right after parsing and validation, and before execution fires: an AST is obviously independant from any execution context so it can be reused over and over again.
>  
> As a proof of concept, I patched our enterprise application with the changes above and ran the benchmark again: we experienced a very significant improvement in response times and CPU usage, between 10% to 30% decreased response times (knowing that these pages also perform time consuming tasks such as database updates and search engine lookups).
> I'm happy to provide a PR for you guys to have a look if you deem it worthy.



--
This message was sent by Atlassian Jira
(v8.20.1#820001)