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

[Vladimir Kozlov]

Tom Rodriguez wrote:
> 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.
> 

My case could be also frequent since javac will
generate SB for the next case:  SB.append("size="+x).toString()

But, I agree, you don't need to implement it now.

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

I see, the next check will fail for my case. And verification code also.

  447     } else if (cnode->method()->holder() == m->holder() &&
  448                cnode->method()->name() == ciSymbol::append_name() &&

OK.

Thanks,
Vladimir



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