Bug 49547 - Minor performance enhancement in set result evaluation
Summary: Minor performance enhancement in set result evaluation
Alias: None
Product: Taglibs
Classification: Unclassified
Component: Standard Taglib (show other bugs)
Version: 1.2.0
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2010-07-03 13:26 UTC by Jeremy Boynes
Modified: 2010-07-03 15:35 UTC (History)
0 users

patch to remove one check and a unneeded call to BodyContent#getString (2.38 KB, patch)
2010-07-03 13:27 UTC, Jeremy Boynes
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Boynes 2010-07-03 13:26:05 UTC
Eliminate one comparison when evaluating body content as the content of "value" attribute can always be used when it has been specified.

Jasper's implementation of BodyContent#getString() constructs a new value each time so only call it once to avoid an unnecessary object creation.
Comment 1 Jeremy Boynes 2010-07-03 13:27:20 UTC
Created attachment 25692 [details]
patch to remove one check and a unneeded call to BodyContent#getString
Comment 2 Henri Yandell 2010-07-03 14:13:05 UTC
There's a functionality change here:

If valueSpecified is false, yet there is a value, it used to return value. Now it returns bodyContent based results. 

I don't see this being in the spec, so presumably this is bad design that we're stuck with supporting; a protected status variable in addition to the main one, rather than a 'isValueSpecified()' method. 

Comment 3 Jeremy Boynes 2010-07-03 14:53:00 UTC
value and valueSpecified are both protected variables and part of our implementation rather than the spec. The container will just call setValue(Object) to set the attribute's value and both our subclasses set both variables in their implementation.

My thought was that the contract this has with its subclasses is that both must be set and if that is met the change works. The original tests I added weren't doing that which was why it was OK to change them.
Comment 4 Henri Yandell 2010-07-03 15:35:24 UTC
Agreed that the contract would be that both must be set. 

I've added javadoc that the two attributes must be in sync as a part of this commit:

svn ci -m "Improving the performance of SetSupport by only calling bodyContent.getString() once per Jeremy's patch #49547"Sending        impl/src/main/java/org/apache/taglibs/standard/tag/common/core/SetSupport.java
Sending        impl/src/test/java/org/apache/taglibs/standard/tag/common/core/TestSetSupport.java
Transmitting file data ..
Committed revision 960259.