[
https://issues.apache.org/jira/browse/WW-5147?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart resolved WW-5147.
-------------------------------
Resolution: Fixed
PR got merged, thanks a lot [~davoustp] !
> 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
> Fix For: 2.6
>
> Time Spent: 3h 40m
> Remaining Estimate: 0h
>
> 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, 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)
