David, Thank you very much for the suggestions. I agree a factory method would be faster. But the factory method references a cache (or pool of objects), right?
Btw, I started playing around with the FlexibleStringExpander code. I think I've come up with a better implementation of creating a parse tree. -Adrian --- On Mon, 8/18/08, David E Jones <[EMAIL PROTECTED]> wrote: > From: David E Jones <[EMAIL PROTECTED]> > Subject: Re: Discussion: FlexibleStringExpander > To: [email protected] > Date: Monday, August 18, 2008, 6:03 PM > 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 > >
