Discussion: FlexibleStringExpander refactor

[Adrian Crum]

When I refactored FlexibleStringExpander.java a while ago (rev 687442),
there were things I wanted to improve but couldn't - because I wanted to
maintain backward-compatibility.

Adam's recent memory-saving efforts reminded me of one of the things I
wanted to change in FlexibleStringExpander.java - I wanted to make the
object more lightweight.

OFBiz can hold anywhere from tens of thousands to hundreds of thousands
of these objects in memory, so an object size reduction could be beneficial.

With that under consideration, I would like to do a little more work on
the class. What I have in mind is to keep the existing API the same - to
preserve backward compatibility - but add an interface so that we can
make gradual changes to framework code that will improve memory use.

Here are the changes I have in mind:

1. Extract an interface from FlexibleStringExpander, call it something
like StringExpression.
2. Convert FlexibleStringExpander to an abstract class that implements
StringExpression.
3. Have FlexibleStringExpander's nested classes extend
FlexibleStringExpander.
4. Reduce object size by having each FlexibleStringExpander subclass
contain only the objects it needs.

This is an initial step - nothing changes from the API point of view. No
changes would need to be made to framework code.

As time and resources permit, we can gradually change lines like:

FlexibleStringExpander fse = FlexibleStringExpander.getInstance(expression);

to:

StringExpression se = FlexibleStringExpander.getInstance(expression);

and both versions will work the same.

After all FlexibleStringExpander instances have been replaced with
StringExpression instances, we can extract the FlexibleStringExpander
nested classes into individual files, have them implement
StringExpression, and remove the class extension. FlexibleStringExpander
then becomes nothing more than a factory and some static convenience
methods.

That's it. If you want to know how this will benefit memory use, read on...

Consider this mini-language statement:

<set field="isTrue" value="Y"/>

The value attribute is kept in memory as a FlexibleStringExpander
instance. That instance will contain one instance of ConstElem that
contains the String "Y". So, let's tally up the memory used for this
single character:

FlexibleStringExpander fields:

   String orig = "Y"
   List strElems = a List with one ConstElem element
   int hint = 20 (used as a StringBuilder initial size)

ConstElem fields:

   String str = "Y"

That's a lot of overhead to store a single character. After the proposed
refactor, the value attribute will be kept in memory as a ConstElem
instance - which will contain nothing more than the String "Y".

The ConstElem class really doesn't need the String class methods, so we
can eliminate the String instance too - by storing it as a character
array. Now the "Y" StringExpression instance takes up the same space as
a String object.

What do you think?

-Adrian

[Adam Heath-2]

Adrian Crum wrote:

> When I refactored FlexibleStringExpander.java a while ago (rev 687442),
> there were things I wanted to improve but couldn't - because I wanted to
> maintain backward-compatibility.
>
> Adam's recent memory-saving efforts reminded me of one of the things I
> wanted to change in FlexibleStringExpander.java - I wanted to make the
> object more lightweight.
>
> OFBiz can hold anywhere from tens of thousands to hundreds of thousands
> of these objects in memory, so an object size reduction could be
> beneficial.
>
> With that under consideration, I would like to do a little more work on
> the class. What I have in mind is to keep the existing API the same - to
> preserve backward compatibility - but add an interface so that we can
> make gradual changes to framework code that will improve memory use.

Er, no.  Rewrites for the sole purpose of reducing memory are bad.
Try to profile ofbiz first.  All the stuff I recently did was the
direct outcome of inspecting memory dumps.

For reference, the tools I used were jmap, jhat, and ibm heap analyzer.

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>> When I refactored FlexibleStringExpander.java a while ago (rev 687442),
>> there were things I wanted to improve but couldn't - because I wanted to
>> maintain backward-compatibility.
>>
>> Adam's recent memory-saving efforts reminded me of one of the things I
>> wanted to change in FlexibleStringExpander.java - I wanted to make the
>> object more lightweight.
>>
>> OFBiz can hold anywhere from tens of thousands to hundreds of thousands
>> of these objects in memory, so an object size reduction could be
>> beneficial.
>>
>> With that under consideration, I would like to do a little more work on
>> the class. What I have in mind is to keep the existing API the same - to
>> preserve backward compatibility - but add an interface so that we can
>> make gradual changes to framework code that will improve memory use.
>
> Er, no.  Rewrites for the sole purpose of reducing memory are bad.
> Try to profile ofbiz first.  All the stuff I recently did was the
> direct outcome of inspecting memory dumps.

So those rewrites were NOT for the sole purpose of reducing memory?

[Adam Heath-2]

Adrian Crum wrote:

> That's it. If you want to know how this will benefit memory use, read on...
>
> Consider this mini-language statement:
>
> <set field="isTrue" value="Y"/>
>
> The value attribute is kept in memory as a FlexibleStringExpander
> instance. That instance will contain one instance of ConstElem that
> contains the String "Y". So, let's tally up the memory used for this
> single character:
>
> FlexibleStringExpander fields:
>
>   String orig = "Y"
>   List strElems = a List with one ConstElem element
>   int hint = 20 (used as a StringBuilder initial size)

ArrayList has the internal array, which has a default size of 20, you
are correct.  However, that's not the only field.  There is an 'int
size' and 'int modCount'.

> ConstElem fields:
>
>   String str = "Y"
>
> That's a lot of overhead to store a single character. After the proposed
> refactor, the value attribute will be kept in memory as a ConstElem
> instance - which will contain nothing more than the String "Y".
>
> The ConstElem class really doesn't need the String class methods, so we
> can eliminate the String instance too - by storing it as a character
> array. Now the "Y" StringExpression instance takes up the same space as
> a String object.

The embed String meanwhile has an 'int hash', 'int count', 'int
offset' field, in addition to the 'char[] value' array.

[Adam Heath-2]

Adrian Crum wrote:

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> When I refactored FlexibleStringExpander.java a while ago (rev 687442),
>>> there were things I wanted to improve but couldn't - because I wanted to
>>> maintain backward-compatibility.
>>>
>>> Adam's recent memory-saving efforts reminded me of one of the things I
>>> wanted to change in FlexibleStringExpander.java - I wanted to make the
>>> object more lightweight.
>>>
>>> OFBiz can hold anywhere from tens of thousands to hundreds of thousands
>>> of these objects in memory, so an object size reduction could be
>>> beneficial.
>>>
>>> With that under consideration, I would like to do a little more work on
>>> the class. What I have in mind is to keep the existing API the same - to
>>> preserve backward compatibility - but add an interface so that we can
>>> make gradual changes to framework code that will improve memory use.
>>
>> Er, no.  Rewrites for the sole purpose of reducing memory are bad.
>> Try to profile ofbiz first.  All the stuff I recently did was the
>> direct outcome of inspecting memory dumps.
>
> So those rewrites were NOT for the sole purpose of reducing memory?

Er, I mean to say that don't just assume that you need to rewwrite it.

I'm looking at the memory usage in heap analyzer right now.

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>> That's it. If you want to know how this will benefit memory use, read on...
>>
>> Consider this mini-language statement:
>>
>> <set field="isTrue" value="Y"/>
>>
>> The value attribute is kept in memory as a FlexibleStringExpander
>> instance. That instance will contain one instance of ConstElem that
>> contains the String "Y". So, let's tally up the memory used for this
>> single character:
>>
>> FlexibleStringExpander fields:
>>
>>   String orig = "Y"
>>   List strElems = a List with one ConstElem element
>>   int hint = 20 (used as a StringBuilder initial size)
>
> ArrayList has the internal array, which has a default size of 20, you
> are correct.  However, that's not the only field.  There is an 'int
> size' and 'int modCount'.
>
>> ConstElem fields:
>>
>>   String str = "Y"
>>
>> That's a lot of overhead to store a single character. After the proposed
>> refactor, the value attribute will be kept in memory as a ConstElem
>> instance - which will contain nothing more than the String "Y".
>>
>> The ConstElem class really doesn't need the String class methods, so we
>> can eliminate the String instance too - by storing it as a character
>> array. Now the "Y" StringExpression instance takes up the same space as
>> a String object.
>
> The embed String meanwhile has an 'int hash', 'int count', 'int
> offset' field, in addition to the 'char[] value' array.

The point I was trying to make is: We can have an expression evaluator
for the same cost as a String. If it costs less than s String, then so
much the better.

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> When I refactored FlexibleStringExpander.java a while ago (rev 687442),
>>>> there were things I wanted to improve but couldn't - because I wanted to
>>>> maintain backward-compatibility.
>>>>
>>>> Adam's recent memory-saving efforts reminded me of one of the things I
>>>> wanted to change in FlexibleStringExpander.java - I wanted to make the
>>>> object more lightweight.
>>>>
>>>> OFBiz can hold anywhere from tens of thousands to hundreds of thousands
>>>> of these objects in memory, so an object size reduction could be
>>>> beneficial.
>>>>
>>>> With that under consideration, I would like to do a little more work on
>>>> the class. What I have in mind is to keep the existing API the same - to
>>>> preserve backward compatibility - but add an interface so that we can
>>>> make gradual changes to framework code that will improve memory use.
>>> Er, no.  Rewrites for the sole purpose of reducing memory are bad.
>>> Try to profile ofbiz first.  All the stuff I recently did was the
>>> direct outcome of inspecting memory dumps.
>> So those rewrites were NOT for the sole purpose of reducing memory?
>
> Er, I mean to say that don't just assume that you need to rewwrite it.
>
> I'm looking at the memory usage in heap analyzer right now.

I never assume. I evaluated it back when I did the original refactor -
which was done, by the way, for the sole purpose of reducing memory use.

[Adam Heath-2]

Adrian Crum wrote:

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> Adrian Crum wrote:
>>>>> When I refactored FlexibleStringExpander.java a while ago (rev
>>>>> 687442),
>>>>> there were things I wanted to improve but couldn't - because I
>>>>> wanted to
>>>>> maintain backward-compatibility.
>>>>>
>>>>> Adam's recent memory-saving efforts reminded me of one of the things I
>>>>> wanted to change in FlexibleStringExpander.java - I wanted to make the
>>>>> object more lightweight.
>>>>>
>>>>> OFBiz can hold anywhere from tens of thousands to hundreds of
>>>>> thousands
>>>>> of these objects in memory, so an object size reduction could be
>>>>> beneficial.
>>>>>
>>>>> With that under consideration, I would like to do a little more
>>>>> work on
>>>>> the class. What I have in mind is to keep the existing API the same
>>>>> - to
>>>>> preserve backward compatibility - but add an interface so that we can
>>>>> make gradual changes to framework code that will improve memory use.
>>>> Er, no.  Rewrites for the sole purpose of reducing memory are bad.
>>>> Try to profile ofbiz first.  All the stuff I recently did was the
>>>> direct outcome of inspecting memory dumps.
>>> So those rewrites were NOT for the sole purpose of reducing memory?
>>
>> Er, I mean to say that don't just assume that you need to rewwrite it.
>>
>> I'm looking at the memory usage in heap analyzer right now.
>
> I never assume. I evaluated it back when I did the original refactor -
> which was done, by the way, for the sole purpose of reducing memory use.

Well, I just ran trunk, went straight to webtools, artifact info(got
an exception), then dumped the heap with jmap, and looked at the file
with heap analyzer.  I see 4749 instances in FSE's cache.  All the
instances together take up only 426k of memory.  That's not a whole
lot, considering that the total heap size of this dump is 200M.

I would say that doing this rewrite, if the goal is to reduce memory,
is not worth.  The gains don't justify the means.

However, if you want to do the rewrite to make the code more asthetic,
more perfect, then that is ok, and I like the api you suggested.

[Adrian Crum-2]

--- On Thu, 1/28/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: Discussion: FlexibleStringExpander refactor
> To: [hidden email]
> Date: Thursday, January 28, 2010, 4:39 PM
> Adrian Crum wrote:
> > Adam Heath wrote:
> >> Adrian Crum wrote:
> >>> Adam Heath wrote:
> >>>> Adrian Crum wrote:
> >>>>> When I refactored
> FlexibleStringExpander.java a while ago (rev
> >>>>> 687442),
> >>>>> there were things I wanted to improve
> but couldn't - because I
> >>>>> wanted to
> >>>>> maintain backward-compatibility.
> >>>>>
> >>>>> Adam's recent memory-saving efforts
> reminded me of one of the things I
> >>>>> wanted to change in
> FlexibleStringExpander.java - I wanted to make the
> >>>>> object more lightweight.
> >>>>>
> >>>>> OFBiz can hold anywhere from tens of
> thousands to hundreds of
> >>>>> thousands
> >>>>> of these objects in memory, so an
> object size reduction could be
> >>>>> beneficial.
> >>>>>
> >>>>> With that under consideration, I would
> like to do a little more
> >>>>> work on
> >>>>> the class. What I have in mind is to
> keep the existing API the same
> >>>>> - to
> >>>>> preserve backward compatibility - but
> add an interface so that we can
> >>>>> make gradual changes to framework code
> that will improve memory use.
> >>>> Er, no.  Rewrites for the sole
> purpose of reducing memory are bad.
> >>>> Try to profile ofbiz first.  All the
> stuff I recently did was the
> >>>> direct outcome of inspecting memory
> dumps.
> >>> So those rewrites were NOT for the sole
> purpose of reducing memory?
> >>
> >> Er, I mean to say that don't just assume that you
> need to rewwrite it.
> >>
> >> I'm looking at the memory usage in heap analyzer
> right now.
> >
> > I never assume. I evaluated it back when I did the
> original refactor -
> > which was done, by the way, for the sole purpose of
> reducing memory use.
>
> Well, I just ran trunk, went straight to webtools, artifact
> info(got
> an exception), then dumped the heap with jmap, and looked
> at the file
> with heap analyzer.  I see 4749 instances in FSE's
> cache.  All the
> instances together take up only 426k of memory. 
> That's not a whole
> lot, considering that the total heap size of this dump is
> 200M.

Agreed - the cached expressions are not the memory wasters. The example I gave does not get cached.

[Adrian Crum]

Adrian Crum wrote:

> --- On Thu, 1/28/10, Adam Heath <[hidden email]> wrote:
>
>> From: Adam Heath <[hidden email]>
>> Subject: Re: Discussion: FlexibleStringExpander refactor
>> To: [hidden email]
>> Date: Thursday, January 28, 2010, 4:39 PM
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> Adrian Crum wrote:
>>>>> Adam Heath wrote:
>>>>>> Adrian Crum wrote:
>>>>>>> When I refactored
>> FlexibleStringExpander.java a while ago (rev
>>>>>>> 687442),
>>>>>>> there were things I wanted to improve
>> but couldn't - because I
>>>>>>> wanted to
>>>>>>> maintain backward-compatibility.
>>>>>>>
>>>>>>> Adam's recent memory-saving efforts
>> reminded me of one of the things I
>>>>>>> wanted to change in
>> FlexibleStringExpander.java - I wanted to make the
>>>>>>> object more lightweight.
>>>>>>>
>>>>>>> OFBiz can hold anywhere from tens of
>> thousands to hundreds of
>>>>>>> thousands
>>>>>>> of these objects in memory, so an
>> object size reduction could be
>>>>>>> beneficial.
>>>>>>>
>>>>>>> With that under consideration, I would
>> like to do a little more
>>>>>>> work on
>>>>>>> the class. What I have in mind is to
>> keep the existing API the same
>>>>>>> - to
>>>>>>> preserve backward compatibility - but
>> add an interface so that we can
>>>>>>> make gradual changes to framework code
>> that will improve memory use.
>>>>>> Er, no.  Rewrites for the sole
>> purpose of reducing memory are bad.
>>>>>> Try to profile ofbiz first.  All the
>> stuff I recently did was the
>>>>>> direct outcome of inspecting memory
>> dumps.
>>>>> So those rewrites were NOT for the sole
>> purpose of reducing memory?
>>>> Er, I mean to say that don't just assume that you
>> need to rewwrite it.
>>>> I'm looking at the memory usage in heap analyzer
>> right now.
>>> I never assume. I evaluated it back when I did the
>> original refactor -
>>> which was done, by the way, for the sole purpose of
>> reducing memory use.
>>
>> Well, I just ran trunk, went straight to webtools, artifact
>> info(got
>> an exception), then dumped the heap with jmap, and looked
>> at the file
>> with heap analyzer.  I see 4749 instances in FSE's
>> cache.  All the
>> instances together take up only 426k of memory.
>> That's not a whole
>> lot, considering that the total heap size of this dump is
>> 200M.
>
> Agreed - the cached expressions are not the memory wasters. The example I gave does not get cached.

I just went to the artifact info page and counted up 78401 non-cached
instances of FlexibleStringExpander. So, we could save a few megabytes
with this change.

By the way, I was able to achieve the results I wanted without the
interface or having to change any additional framework code. I will
probably commit this after I've had more time to test it.

-Adrian