OFBiz › OFBiz - Dev

Discussion: FlexibleStringExpander

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

6 messages Options

Adrian Crum

Discussion: FlexibleStringExpander

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

David E Jones

Re: Discussion: FlexibleStringExpander

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
>

Adam Heath-2

Re: Discussion: FlexibleStringExpander

David E Jones wrote:

>
> 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.

Most excellent.

I'm working on changing service engines to cache invocations. Doing a
loadClass, getMethod each time is slow; caching the looked up field is a
significant speedup.

The biggest issue is that each engine and modelservice is not tied
tightly; I'd like to have the model do the invocation, so that the
cached invoker(whether it is a reflected method, or some other more
advanced construct) can be cached and auto-cleared if the model changes.

Adrian Crum-2

Re: Discussion: FlexibleStringExpander

In reply to this post by David E Jones

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 <[hidden email]> wrote:

> From: David E Jones <[hidden email]>
> Subject: Re: Discussion: FlexibleStringExpander
> To: [hidden email]
> 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
> >

Adam Heath-2

Re: Discussion: FlexibleStringExpander

In reply to this post by Adam Heath-2

Adam Heath wrote:

> David E Jones wrote:
>>
>> 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.
>
> Most excellent.
>
> I'm working on changing service engines to cache invocations. Doing a
> loadClass, getMethod each time is slow; caching the looked up field is a
> significant speedup.

Here's a test. Note how that plain standard reflection, when the Method
lookup is cached in a final instance variable, is *slower* than my own
custom implementation.

The fourth item listed is basically how OfBiz does it's method calls.
Basically, ClassLoader.loadClass(className).getMethod(methodName,
DispatchContext.class, Map.class). Continuously.

I'll be working this into the ServiceEngine over the next few days.

ps: this is the first major speedup work to come from webslinger.

==
ref.ReflectionTest$Normal(500000)=2.539026068
ref.ReflectionTest$Reflection(500000)=3.287744069
ref.ReflectionTest$ContinualReflection(500000)=6.518010998
ref.ReflectionTest$ContinualLoadingReflection(500000)=8.913640043
ref.ReflectionTest$Compiled(500000)=2.871062177
ref.ReflectionTest$Dispatcher(500000)=2.672173324
ref.ReflectionTest$SyncRefLoadingReflection(500000)=3.383238542
ref.ReflectionTest$AtomicRefLoadingReflection(500000)=3.37369552
ref.ReflectionTest$SyncRefLoadingCompiled(500000)=3.041537616
ref.ReflectionTest$AtomicRefLoadingCompiled(500000)=3.027191239
ref.ReflectionTest$SyncRefLoadingDispatcher(500000)=2.784919375
ref.ReflectionTest$AtomicRefLoadingDispatcher(500000)=2.791944651
==

Adrian Crum-2

Re: Discussion: FlexibleStringExpander

In reply to this post by David E Jones

I finished working on the FlexibleStringExpander class and submitted a patch for review - https://issues.apache.org/jira/browse/OFBIZ-1924.

Following the same sequence of steps in my previous test yielded 3798 total cache entries - a considerable savings.

-Adrian

--- On Mon, 8/18/08, David E Jones <[hidden email]> wrote: