The more common approach, and what would be even faster than a static
method with an internal cache, is to use a factory method that is
smart enough to see if an object for the given original String already
exists and if so then use it.
This implies that the FlexibleStringExpander is immutable, which I
looked at real quick and it appears to already be.
I was thinking about using the javolution ObjectFactory to do object
recycling (like in GenericDelegator), but that really isn't needed for
an immutable Object pattern like this where the objects retrieved are
kept and reused and not rebuilt each time rather than being thrown
away and recreated frequently.
BTW, the factory method with an actual instance pointed to in the user
objects is faster than the static method doing a lookup in an internal
cache mostly because it saves on the lookup.
-David
On Aug 18, 2008, at 1:54 PM, Adrian Crum wrote:
Some time ago I had suggested eliminating FlexibleStringExpander
instances and just use the expandString(...) static method instead.
David pointed out that the FlexibleStringExpander instances perform
some pre-parsing of the expression during construction to make the
string expansion faster. Switching to the static method would be a
performance hit. I agreed and I put the idea on the shelf.
Lately I've been thinking about it more and I did some research.
There are 247 protected instances of FlexibleStringExpander inside
other classes. Each of those containing classes might have hundreds
of instances. Many of the FlexibleStringExpander instances contain
an empty expression - they have nothing to expand.
To find out empirically how many FlexibleStringExpander instances
are being created, I created a little test. I modified the
FlexibleStringExpander constructor to count how many instances of
each expression were created. I set the cache properties to never
expire and then started OFBiz. I clicked on each main navigation tab
and each sub navigation tab. In other words, I just displayed the
main page of each component. Here are a few results (out of a list
of hundreds):
Expression Count
-------------- ------
true 627
false 1679
screenlet 101
${description} 142
main-decorator 112
(empty expression) 16811
About half of the expressions tallied had only one instance. The
test results were repeatable - restarting OFBiz and following the
same actions produced the exact same results.
The results are enlightening. In the above list, only one of the
expressions needs expansion. 19330 of those FlexibleStringExpander
instances aren't needed.
Keep in mind that this was from displaying one page per component.
In actual use these numbers would be much higher.
So, getting back to my original static method idea...
What if we made the static method smarter? It first scans the
expression for "${" and if it isn't found (nothing to expand) it
returns the original string. If the expression needs expansion, it
looks up the expression in an "expression cache." If the expression
isn't found in the cache, the expression is pre-parsed and placed in
the cache. The pre-parsed expression is expanded and returned.
Now the only objects being created and saved in memory are cache
entries for expressions that require expansion. Using the results
list above, we would have one cache entry (${description}) instead
of 19472 FlexibleStringExpander instances.
I'm not really pushing this idea - I'm just offering it up for
comment. We can't really know the net results unless a lot of re-
coding is done.
Comments are welcome.
-Adrian
