review (M) for 6892658: C2 should optimize some stringbuilder patterns

[Tom Rodriguez]

On Nov 11, 2009, at 10:36 AM, Vladimir Kozlov wrote:

> 
> 
> Tom Rodriguez wrote:
>>> Also _begin instead of other->_begin
>>> in result->set_allocation(other->_begin)
>> _begin is must be the earliest JVMState of the pattern and other->_begin has to be earlier than _begin otherwise the couldn't be merged so I can't just swap them around.
> 
> Then I don't get how you optimize next code:
> 
> SB.append((new SB()).append(s).toString()).toString()

It won't handle that as it's currently constructed but it handles

String s = new SB().append().append().toString();
String s2 = new SB().append().append(s).toString();

which is a case we actually care about.  Handling the case you illustrate would require extending the logic in build_candidate quite a bit I think.  I think there are more complex SB pattern that we might like to get but this is currently targeting basic ones.  We can add more later.

> and I don't see any checks that other->_begin dominates _begin.

It's by construction.  Each string concat is a linear piece of control flow from the toString back to the allocation with nothing unknown in between.  We identify a stacking opportunity by detecting that one StringConcat is an argument to another.  Then we merge them together and verify that they still form a closed graph.  That will only be true if they form another linear sequence so other->_begin must dominate _begin.

tom

> 
>>> There are several places where you do next check,
>>> may be you can factor it in a separate function:
>>> 
>>> method->holder() == C->env()->StringBuilder_klass() ||
>>> method->holder() == C->env()->StringBuffer_klass()
>> I'm not sure factoring it out would be better.
> 
> OK.
> 
>>> May be also verify has_stringbuilder() in PhaseStringOpts().
>> Why?
> 
> OK, I see that caller code of PhaseStringOpts() has has_stringbuilder()
> 
>>> Also this coalesce code will not work if "other" is used by
>>> several sc/arguments since you removed it from the list after
>>> first match and merge. For example:
>>> 
>>> String s0 = new SB().append(1)...toString();
>>> String s1 = new SB().append(s0).append(s0).toString();
>>> 
>>> I would keep it and always replace "c" with merged
>>> (you need to modify StringConcat::merge() as I pointed above).
>>> The "o" will be removed automatically if there are no other uses.
>> I don't want to support that.  I don't think that's an interesting pattern.  It would also require rewriting the management of the control and trap lists and I don't want to get into that.
> 
> OK.
> 
> Vladimir
> 
>>> I will look on copy_string() and related methods tomorrow.
>> Thanks.
>> tom
>>> Vladimir