Bug 58327 - ValueExpressionLiteral should cache the string value
Summary: ValueExpressionLiteral should cache the string value
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: EL (show other bugs)
Version: 8.0.26
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2015-09-04 09:46 UTC by Andreas Kohn
Modified: 2015-09-04 11:31 UTC (History)
0 users

Proposed patch (775 bytes, patch)
2015-09-04 09:46 UTC, Andreas Kohn
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kohn 2015-09-04 09:46:40 UTC
Created attachment 33061 [details]
Proposed patch

ValueExpressionLiteral invokes #getExpressionString() twice for many methods to notify the context.

Calculating the string value can be expensive, for example in my case my values are rather "big" JSON wrapper objects. For a literal value this string value should be cacheable - the thing is read-only and a literal value, after all.

Attached a proposed patch to implement that - but I'm not sure whether there might be specific tests to run or spec issues preventing this.

For the time being I worked around the issue in our own code by using a different ValueExpression that has this caching behavior.
Comment 1 Mark Thomas 2015-09-04 11:17:46 UTC
The only risk I can see is that even though the ValueExpression is read-only, the object it wraps is not and that object may change over time. That may change the result of its toString() method which in turn would impact the result of #getExpressionString(). However, the use of toString() here is an implementation detail internal to Tomcat. The spec has no expectations of what should be produced for #getExpressionString() in this case.

On balance I think the potential performance benefits outweigh the possible confusion that might be caused by caching the toString() value if getExpressionString() is used for debugging. I'll look at getting this patch applied.
Comment 2 Mark Thomas 2015-09-04 11:31:32 UTC
Thanks for the report and the patch.

It has been applied to trunk and 8.0.x (for 8.0.27 onwards). I haven't applied it to 7.0.x since getExpressionString() is not used so extensively in 7.0.x.