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

[Tom Rodriguez]

On Nov 10, 2009, at 6:13 PM, Vladimir Kozlov wrote:

> 
> Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6892658/
> 
> stringopts.cpp
> 
> It would be nice to have all fields description in StringConcat class.

Added.

> In the changes description you said that new String(SB.toString())
> is processed also but the code is under #if 0 (merge_add()).

I decided there were some possible correctness issues with the logic proving that it wasn't used anywhere else so I pulled it out.  I'll restore it later.  

> 
> Change PrintOptimizeStringConcat to "notproduct" flag since
> it is used only under #ifndef PRODUCT.

Ok.

> In StringConcat::merge()
> _stringopt could be used instead of other->_stringopts
> in  new StringConcat(other->_stringopts, _end)

Sure.

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

> It will allow to merge "other" several times see my comment bellow
> about coalesce concats.

I'm certainly not going to incorporate any extensions in it at this point and I'm a little dubious on the utility of handling that pattern as well.

> 
> In StringConcat::eliminate_initialize() would be nice
> to have assert that initialize node doesn't have raw stores:
> assert(init->req() <= InitializeNode::RawStores,"");

Ok.

> In build_candidate() you may use recv->uncast() instead of:
> 
> 366     if (recv->Opcode() == Op_CastPP) {
> 367       recv = recv->in(1);
> 368     }

Ok.

> At the same code you skip Proj so you may fail AllocateNode::Ideal_allocation()
> since it expects Proj or CheckCastPP nodes. Just check that recv->is_Allocate().

Ok.

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

> May be also verify has_stringbuilder() in PhaseStringOpts().

Why?

> 
> I don't understand next break code.
> 
> 562                 c = 0;
> 563                 break;
> 
> You have 3 nested loops so the next break will return to the
> second loop, not first, so "sc" will not be updated and
> o==0 will be skipped. Why?

I'd intended to restart the search at beginning but you're right that it's not restarting the way I want.  I've switched to a goto to the head of the first loop since we don't have labeled breaks.

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

> 
> I will look on copy_string() and related methods tomorrow.

Thanks.

tom

> 
> Vladimir