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

[Vladimir Kozlov]

Tom Rodriguez wrote:
> http://cr.openjdk.java.net/~never/6892658/

stringopts.cpp

It would be nice to have all fields description in StringConcat class.

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

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

In StringConcat::merge()
_stringopt could be used instead of other->_stringopts
in  new StringConcat(other->_stringopts, _end)
Also _begin instead of other->_begin
in result->set_allocation(other->_begin)
It will allow to merge "other" several times see my comment bellow
about coalesce concats.

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

In build_candidate() you may use recv->uncast() instead of:

  366     if (recv->Opcode() == Op_CastPP) {
  367       recv = recv->in(1);
  368     }

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

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

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

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?

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 will look on copy_string() and related methods tomorrow.

Vladimir