[
https://issues.apache.org/jira/browse/WW-5147?focusedWorklogId=681089&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-681089
]
ASF GitHub Bot logged work on WW-5147:
--------------------------------------
Author: ASF GitHub Bot
Created on: 13/Nov/21 10:39
Start Date: 13/Nov/21 10:39
Worklog Time Spent: 10m
Work Description: davoustp commented on pull request #504:
URL: https://github.com/apache/struts/pull/504#issuecomment-968036028
> Thanks for producing this PR in response to the JIRA issue. 👍
My pleasure, that's normal to contribute back when we find something useful
for the community (OSS rocks).
> Looking at the history of OgnlUtil (as far back as I can see in the
repository), it looks like the logic considered an expression "valid" to put in
the expression cache if-and-only-if it could be successfully executed. It seems
like that may have been an intentional decision based on the original code
comment ?
Yep, that's my belief as well, the logic/behaviour is not documented,
neither as a comment nor in commit history, so we're stuck in a guess game.
> There may be a trade-off in execution performance (by allowing more
expressions to be considered "valid" and cached) and memory utilization (due to
the larger number of potential expressions now being cached).
Very true, as with any caching strategy: you trade off CPU or latency
against memory.
> Having expressions considered "valid" if-and-only-if they can be executed
successfully may have provided a limiting factor to keep the cache size more
manageable in the past, but that is just my speculation.
Definitely. However, I do think that this is a weird way to implement memory
usage control.
The problem I see is that the fact that an expression evaluates successfully
or not does *not* depend on the expression itself, but on the context it's
evaluated against.
Such an expression may fail to execute 1000 times, and succeeds on the next
attempt (because using a different context which allows the expression to
evaluate successfully). So all in a sudden, the expression gets cached...
All in all, the condition to cache a valid expression entirely depends onto
the context we throw at it.
Weird, isn't it?
If we care about memory control, I would then rather go using a real cache
with a proper eviction policy, such as https://github.com/ben-manes/caffeine (I
use it a lot, it's rock stable, highly efficient and extremely flexible).
This would allow to evict least frequently used expressions, which can even
be balanced against its cost to build if needed. This could be bounded or not,
time-based...
(we'd need to check the minimum JDK version supported by Struts2, though,
since caffeine requires JDK 8 minimum if I remember correctly)
This way, any application generating a very large set of expressions would
still get the benefit of caching for the more frequently used expressions
without growing the cache too large. And expressions get cached irrespective of
any evaluation result.
A small set of configuration knobs might be required as well (to allow user
control on cache retention duration, bounded/unbounded and upper bound if
needed...).
And going this way could only go into the 2.6.x branch of course.
> This change may be safer to attempt for the 2.6.x branch (as opposed to PR
#503 for 2.5.x), since it introduces a new behaviour for the expression cache.
If we prefer keeping the changes low, yes, of course, we could have that
change in 2.6.x only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 681089)
Time Spent: 2h 20m (was: 2h 10m)
> 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
>
> Time Spent: 2h 20m
> 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)
