You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@struts.apache.org by "Pascal Davoust (Jira)" <ji...@apache.org> on 2021/11/10 08:47:00 UTC
[jira] [Updated] (WW-5147) OGNL valid expression is not cached and
is parsed over again in some situations
[ https://issues.apache.org/jira/browse/WW-5147?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Pascal Davoust updated WW-5147:
-------------------------------
Description:
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.
was:
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.
> 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)