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/11 10:36:00 UTC
[jira] [Comment Edited] (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:comment-tabpanel&focusedCommentId=17441772#comment-17441772 ]
Pascal Davoust edited comment on WW-5147 at 11/11/21, 10:35 AM:
----------------------------------------------------------------
I just submitted PRs https://github.com/apache/struts/pull/503 (2.5.x) and https://github.com/apache/struts/pull/504 (2.6.x).
was (Author: davoustp):
I just submitted PRs #503 (2.5.x) and #504 (2.6.x).
> 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, 2.5.28
>
>
> 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)