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

[Tom Rodriguez]

http://cr.openjdk.java.net/~never/6892658/

[Andreas Kohn-5]

On Mon, 2009-11-09 at 15:46 -0800, Tom Rodriguez wrote:
> http://cr.openjdk.java.net/~never/6892658/
Hi,

more question a question than a review: should match_F_Y() not require
synchronized?

 306 inline bool match_F_Y(jshort flags) {
 307   const int req = 0;
                     = JVM_ACC_SYNCHRONIZED; ?
 308   const int neg = JVM_ACC_STATIC;
 309   return (flags & (req | neg)) == req;
 310 }

I'm just a casual reader, so I might be missing something here.

Regards,
--
Andreas

--
Never attribute to malice that which can be adequately explained by
stupidity.                                        -- Hanlon's Razor

[Christian Thalinger-3]

On Mon, 2009-11-09 at 15:46 -0800, Tom Rodriguez wrote:
> http://cr.openjdk.java.net/~never/6892658/

src/share/vm/memory/universe.hpp:

+   static oop          _the_MIN_VALUE_string;          // A cache of "" as a Java string

The min-value is missing in the comment.


src/share/vm/opto/callGenerator.cpp:

void WarmCallInfo::make_hot() {

Why is this method now Unimplemented()?


src/share/vm/opto/callnode.cpp:

+   // The resproj may not exist because the result couuld be ignored
+   // and the exception object may not be exist if an exception handler

Typo.


src/share/vm/opto/compile.cpp:

+         // Separate projections were use for the exception path which

"were used"?


src/share/vm/opto/phase.hpp:

      LIVE,                       // Dragon-book LIVE range problem
+     StringOpts,
      Interference_Graph,         // Building the IFG

A comment would be nice.


src/share/vm/utilities/growableArray.hpp:

+   /* inserts the given element before the element at index i */                      

A C comment?

I will look at src/share/vm/opto/stringopts.cpp tomorrow.

-- Christian

[Tom Rodriguez]

No, there's definitely something wrong.  It worked to match what I wanted to so I didn't look that closely but it will certainly match a broader range of things than it should.  I would expect the match functions to be disjoint from each other but both F_R and F_S ignore the JVM_ACC_NATIVE bit so F_RY matches F_R and F_SN matches F_S though the reverse isn't true.

Anyway, thanks for catching this.  I've fixed match_F_Y as you indicated and updated the flags enum to more clearly report the indeterminate nature of the native bit in some of the types.

    F_none = 0,
    F_R,                        // !static ?native !synchronized (R="regular")                              
    F_S,                        //  static ?native !synchronized                                            
    F_Y,                        // !static ?native  synchronized                                            
    F_RN,                       // !static  native !synchronized                                            
    F_SN,                       //  static  native !synchronized                                            
    F_RNY                       // !static  native  synchronized

I may file a separate bug to add testing of JVM_ACC_NATIVE to the one which are currently ignoring it.

tom

On Nov 10, 2009, at 5:55 AM, Andreas Kohn wrote:

> On Mon, 2009-11-09 at 15:46 -0800, Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6892658/
> Hi,
>
> more question a question than a review: should match_F_Y() not require
> synchronized?
>
> 306 inline bool match_F_Y(jshort flags) {
> 307   const int req = 0;
>                     = JVM_ACC_SYNCHRONIZED; ?
> 308   const int neg = JVM_ACC_STATIC;
> 309   return (flags & (req | neg)) == req;
> 310 }
>
> I'm just a casual reader, so I might be missing something here.
>
> Regards,
> --
> Andreas
>
> --
> Never attribute to malice that which can be adequately explained by
> stupidity.                                        -- Hanlon's Razor

[Tom Rodriguez]

On Nov 10, 2009, at 11:42 AM, Christian Thalinger wrote:

> On Mon, 2009-11-09 at 15:46 -0800, Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6892658/
>
> src/share/vm/memory/universe.hpp:
>
> +   static oop          _the_MIN_VALUE_string;          // A cache of "" as a Java string
>
> The min-value is missing in the comment.

Fixed.

> src/share/vm/opto/callGenerator.cpp:
>
> void WarmCallInfo::make_hot() {
>
> Why is this method now Unimplemented()?

make_hot is a non-working implementation of late inlining and long term I think all that code should be deleted and rebuilt if we intend to move more generally to a late inlining strategy.  I just went ahead and deleted that for now.  I could leave it behind if you like.

>
>
> src/share/vm/opto/callnode.cpp:
>
> +   // The resproj may not exist because the result couuld be ignored
> +   // and the exception object may not be exist if an exception handler
>
> Typo.

Fixed.

>
>
> src/share/vm/opto/compile.cpp:
>
> +         // Separate projections were use for the exception path which
>
> "were used"?

Fixed.

>
>
> src/share/vm/opto/phase.hpp:
>
>      LIVE,                       // Dragon-book LIVE range problem
> +     StringOpts,
>      Interference_Graph,         // Building the IFG
>
> A comment would be nice.

Fixed.

> src/share/vm/utilities/growableArray.hpp:
>
> +   /* inserts the given element before the element at index i */                      
>
> A C comment?

It started as a copy paste from a context that couldn't use C++ style comments and I never fixed it.  I've corrected it.

tom

>
> I will look at src/share/vm/opto/stringopts.cpp tomorrow.
>
> -- Christian
>

[Christian Thalinger-3]

On Tue, 2009-11-10 at 13:15 -0800, Tom Rodriguez wrote:

> > src/share/vm/opto/callGenerator.cpp:
> >
> > void WarmCallInfo::make_hot() {
> >
> > Why is this method now Unimplemented()?
>
> make_hot is a non-working implementation of late inlining and long
> term I think all that code should be deleted and rebuilt if we intend
> to move more generally to a late inlining strategy.  I just went ahead
> and deleted that for now.  I could leave it behind if you like.

No, I just wanted to have a context here.  Thanks.  -- Christian

[Vladimir Kozlov-5]

Tom,

here is review of all files but 2 new, I am looking on them next

the_MIN_VALUE_string may be should be the_MIN_INT_VALUE_string

universe.hpp
static oop _the_MIN_VALUE_string;  // A cache of "" as a Java string
                                                   ^ "-2147483648"

c2_globals.hpp
I thought we will use experimental for OptimizeStringConcat.

callGenerator.cpp
missing _call_node(NULL) in DirectCallGenerator() constructor.

   bool _separate_io_proj; <- add field's descriptor by copying the comment
                              from LateInlineCallGenerator::generate()

do_late_inline()
add checks to not inline when
call_node()->in(0) == NULL || call_node()->in(0)->is_top()

+   for (uint i1 = 0; i1 < call->req(); i1++) {
                            ^ size

callnode.cpp
You copied code from PhaseMacroExpand::extract_call_projections().
Can you use your new method in macro.cpp also? At least for HS17 changes
when you will have more time.

compile.cpp
In gvn_replace_by() why there is no initial_gvn()->hash_insert(use)?

Instead of OptimizeStringConcat && has_stringbuilder() and OptimizeStringConcat
checks may be we can use only has_stringbuilder() (or different name for query method)
and check OptimizeStringConcat in parseHelper.cpp where has_stringbuilder is set?

doCall.cpp
I saw cases when append methods were not inlined because they were
already compiled into "big" compiled method. You call for_late_inline()
after ok_to_inline(), so may be we should relax that condition for
OptimizeStringConcat case.

Should you check that safepoint has > jvms->argoff() inputs?:
Node* receiver = jvms->map()->in(jvms->argoff() + 1);

macro.cpp
Can you print array only when it is array?:
+     log->head("eliminate_allocation %s type='%d'",
+               alloc->is_AllocateArray() ? "array" : "", log->identify(tklass->klass()));

the same with "lock":"unlock"

Vladimir

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

[Vladimir Kozlov-5]

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

[Tom Rodriguez]

On Nov 10, 2009, at 2:11 PM, Vladimir Kozlov wrote:

> Tom,
>
> here is review of all files but 2 new, I am looking on them next
>
> the_MIN_VALUE_string may be should be the_MIN_INT_VALUE_string

I was trying to mimic Integer.MIN_VALUE.  If you want int in there then how about the_min_jint_string?

> universe.hpp
> static oop _the_MIN_VALUE_string;  // A cache of "" as a Java string
>                                                  ^ "-2147483648"

Fixed.

> c2_globals.hpp
> I thought we will use experimental for OptimizeStringConcat.

I guess we could.  I don't really like experimental much but I'm not completely against it.

> callGenerator.cpp
> missing _call_node(NULL) in DirectCallGenerator() constructor.
>
>  bool _separate_io_proj; <- add field's descriptor by copying the comment
>                             from LateInlineCallGenerator::generate()

Ok.

>
> do_late_inline()
> add checks to not inline when
> call_node()->in(0) == NULL || call_node()->in(0)->is_top()

Ok

>
> +   for (uint i1 = 0; i1 < call->req(); i1++) {
>                           ^ size

Ok.

> callnode.cpp
> You copied code from PhaseMacroExpand::extract_call_projections().
> Can you use your new method in macro.cpp also? At least for HS17 changes
> when you will have more time.

I'll make that change later.

> compile.cpp
> In gvn_replace_by() why there is no initial_gvn()->hash_insert(use)?

It was an oversight.  I've added it.  The code should roughly mirror Node::subsume_node.

> Instead of OptimizeStringConcat && has_stringbuilder() and OptimizeStringConcat
> checks may be we can use only has_stringbuilder() (or different name for query method)
> and check OptimizeStringConcat in parseHelper.cpp where has_stringbuilder is set?

I'd be ok with that.

>
> doCall.cpp
> I saw cases when append methods were not inlined because they were
> already compiled into "big" compiled method. You call for_late_inline()
> after ok_to_inline(), so may be we should relax that condition for
> OptimizeStringConcat case.

The code doesn't care whether the interesting methods are inlined or not.  If the methods would have been inlined then we register a late inline for them.  If they wouldn't have been inlined then we tell the DirectCallGenerator to use separate io projs so that we can properly find all the edges if we need to replace it.  THe late inlining logic is mainly about giving us the same result if we don't end up performing the optimization.

> Should you check that safepoint has > jvms->argoff() inputs?:
> Node* receiver = jvms->map()->in(jvms->argoff() + 1);

I guess I could but if there aren't enough then the call itself is malformed and we'll die later won't we?  Do you think i should?

>
> macro.cpp
> Can you print array only when it is array?:
> +     log->head("eliminate_allocation %s type='%d'",
> +               alloc->is_AllocateArray() ? "array" : "", log->identify(tklass->klass()));
>
> the same with "lock":"unlock"

I could but that's not very good xml.  I think I'll just leave that out since arrayness should be discerned from the klass.  I'd prefer to leave it as it for lock though.

tom

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

[Vladimir Kozlov-5]

Tom Rodriguez wrote:
>>
>> the_MIN_VALUE_string may be should be the_MIN_INT_VALUE_string
>
> I was trying to mimic Integer.MIN_VALUE.  If you want int in there then how about the_min_jint_string?
>

Agree with the_min_jint_string.

>
>> c2_globals.hpp
>> I thought we will use experimental for OptimizeStringConcat.
>
> I guess we could.  I don't really like experimental much but I'm not completely against it.
>

I would prefer experimental.

>
>> doCall.cpp
>> I saw cases when append methods were not inlined because they were
>> already compiled into "big" compiled method. You call for_late_inline()
>> after ok_to_inline(), so may be we should relax that condition for
>> OptimizeStringConcat case.
>
> The code doesn't care whether the interesting methods are inlined or not.  If the methods would have been inlined then we register a late inline for them.  If they wouldn't have been inlined then we tell the DirectCallGenerator to use separate io projs so that we can properly find all the edges if we need to replace it.  THe late inlining logic is mainly about giving us the same result if we don't end up performing the optimization.
>

You are right. The code delays inlining, not trying to inline.

>> Should you check that safepoint has > jvms->argoff() inputs?:
>> Node* receiver = jvms->map()->in(jvms->argoff() + 1);
>
> I guess I could but if there aren't enough then the call itself is malformed and we'll die later won't we?  Do you think i should?
>

Leave it as it is.

>> macro.cpp
>> Can you print array only when it is array?:
>> +     log->head("eliminate_allocation %s type='%d'",
>> +               alloc->is_AllocateArray() ? "array" : "", log->identify(tklass->klass()));
>>
>> the same with "lock":"unlock"
>
> I could but that's not very good xml.  I think I'll just leave that out since arrayness should be discerned from the klass.  I'd prefer to leave it as it for lock though.
>

Leave it as it is since it is better for XML.

Thanks,
Vladimir

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

[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

[Tom Rodriguez]

I've updated the webrev with all these changes.

tom

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

> Tom Rodriguez wrote:
>>>
>>> the_MIN_VALUE_string may be should be the_MIN_INT_VALUE_string
>> I was trying to mimic Integer.MIN_VALUE.  If you want int in there then how about the_min_jint_string?
>
> Agree with the_min_jint_string.
>
>>> c2_globals.hpp
>>> I thought we will use experimental for OptimizeStringConcat.
>> I guess we could.  I don't really like experimental much but I'm not completely against it.
>
> I would prefer experimental.
>
>>> doCall.cpp
>>> I saw cases when append methods were not inlined because they were
>>> already compiled into "big" compiled method. You call for_late_inline()
>>> after ok_to_inline(), so may be we should relax that condition for
>>> OptimizeStringConcat case.
>> The code doesn't care whether the interesting methods are inlined or not.  If the methods would have been inlined then we register a late inline for them.  If they wouldn't have been inlined then we tell the DirectCallGenerator to use separate io projs so that we can properly find all the edges if we need to replace it.  THe late inlining logic is mainly about giving us the same result if we don't end up performing the optimization.
>
> You are right. The code delays inlining, not trying to inline.
>
>>> Should you check that safepoint has > jvms->argoff() inputs?:
>>> Node* receiver = jvms->map()->in(jvms->argoff() + 1);
>> I guess I could but if there aren't enough then the call itself is malformed and we'll die later won't we?  Do you think i should?
>
> Leave it as it is.
>
>>> macro.cpp
>>> Can you print array only when it is array?:
>>> +     log->head("eliminate_allocation %s type='%d'",
>>> +               alloc->is_AllocateArray() ? "array" : "", log->identify(tklass->klass()));
>>>
>>> the same with "lock":"unlock"
>> I could but that's not very good xml.  I think I'll just leave that out since arrayness should be discerned from the klass.  I'd prefer to leave it as it for lock though.
>
> Leave it as it is since it is better for XML.
>
> Thanks,
> Vladimir
>
>> tom
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> http://cr.openjdk.java.net/~never/6892658/

[Tom Rodriguez]

Note that using experimental required changes to several globals file because the macro didn't support experimental as is.

tom

On Nov 11, 2009, at 9:11 AM, Tom Rodriguez wrote:

> I've updated the webrev with all these changes.
>
> tom
>
> On Nov 11, 2009, at 8:36 AM, Vladimir Kozlov wrote:
>
>> Tom Rodriguez wrote:
>>>>
>>>> the_MIN_VALUE_string may be should be the_MIN_INT_VALUE_string
>>> I was trying to mimic Integer.MIN_VALUE.  If you want int in there then how about the_min_jint_string?
>>
>> Agree with the_min_jint_string.
>>
>>>> c2_globals.hpp
>>>> I thought we will use experimental for OptimizeStringConcat.
>>> I guess we could.  I don't really like experimental much but I'm not completely against it.
>>
>> I would prefer experimental.
>>
>>>> doCall.cpp
>>>> I saw cases when append methods were not inlined because they were
>>>> already compiled into "big" compiled method. You call for_late_inline()
>>>> after ok_to_inline(), so may be we should relax that condition for
>>>> OptimizeStringConcat case.
>>> The code doesn't care whether the interesting methods are inlined or not.  If the methods would have been inlined then we register a late inline for them.  If they wouldn't have been inlined then we tell the DirectCallGenerator to use separate io projs so that we can properly find all the edges if we need to replace it.  THe late inlining logic is mainly about giving us the same result if we don't end up performing the optimization.
>>
>> You are right. The code delays inlining, not trying to inline.
>>
>>>> Should you check that safepoint has > jvms->argoff() inputs?:
>>>> Node* receiver = jvms->map()->in(jvms->argoff() + 1);
>>> I guess I could but if there aren't enough then the call itself is malformed and we'll die later won't we?  Do you think i should?
>>
>> Leave it as it is.
>>
>>>> macro.cpp
>>>> Can you print array only when it is array?:
>>>> +     log->head("eliminate_allocation %s type='%d'",
>>>> +               alloc->is_AllocateArray() ? "array" : "", log->identify(tklass->klass()));
>>>>
>>>> the same with "lock":"unlock"
>>> I could but that's not very good xml.  I think I'll just leave that out since arrayness should be discerned from the klass.  I'd prefer to leave it as it for lock though.
>>
>> Leave it as it is since it is better for XML.
>>
>> Thanks,
>> Vladimir
>>
>>> tom
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> http://cr.openjdk.java.net/~never/6892658/
>

[Vladimir Kozlov-5]

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

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

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

[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

[Vladimir Kozlov-5]

Final part.

You use kit.gvn(). instead of _gvn-> in several places.
Also I think you can use __ instead of kit. for intcon, makecon, loads
and others and leave it for control, memory operations only.

You use fetch_static_field() only to read Integer.sizeTable. Does it need
to be so generalized? But you can keep it as it is.

Can you separate inlined comments from code by empty lines in int_stringSize()?

In int_stringSize(), I think, TypeAryPtr::INTS memory should be used instead of
TypeAryPtr::CHARS (for final_mem) and int_adr_idx (needs to add it) instead
of char_adr_idx.

In int_getChars() should the sign store to have IfTrue(iff) control?:

1124     Node* st = __ store_to_memory(kit.control(), kit.array_element_address(char_array, m1, T_CHAR),
1125                                   sign, T_CHAR, char_adr_idx);
1126
1127     final_merge->init_req(1, __ IfTrue(iff));


copy_string(), so you not supporting byte array strings for now?
Why 6 is limit for constant strings? Add some comments.


Vladimir

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

[Vladimir Kozlov-5]

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
>

[Tom Rodriguez]

On Nov 11, 2009, at 12:08 PM, Vladimir Kozlov wrote:

> Final part.
>
> You use kit.gvn(). instead of _gvn-> in several places.
> Also I think you can use __ instead of kit. for intcon, makecon, loads
> and others and leave it for control, memory operations only.
>
> You use fetch_static_field() only to read Integer.sizeTable. Does it need
> to be so generalized? But you can keep it as it is.

Originally I was going to read some other fields so I needed something general.  It's based on do_get_xxx and I don't see any reason to simplify it.  I could move it over into GraphKit.

>
> Can you separate inlined comments from code by empty lines in int_stringSize()?

Ok.

> In int_stringSize(), I think, TypeAryPtr::INTS memory should be used instead of
> TypeAryPtr::CHARS (for final_mem) and int_adr_idx (needs to add it) instead
> of char_adr_idx.

Actually there are no stores so it's not needed at all.  I'd added some debugging code that did a runtime call and needed the phi but I think I can remove it completely now.

>
> In int_getChars() should the sign store to have IfTrue(iff) control?:

Ah yes.  It should.  The store in the loop above has a similar problem.

>
> 1124     Node* st = __ store_to_memory(kit.control(), kit.array_element_address(char_array, m1, T_CHAR),
> 1125                                   sign, T_CHAR, char_adr_idx);
> 1126
> 1127     final_merge->init_req(1, __ IfTrue(iff));
>
>
> copy_string(), so you not supporting byte array strings for now?

We don't have byte strings.

> Why 6 is limit for constant strings? Add some comments.

Ok. It's just a number.  6 seems like an ok code space vs. speed tradeoff.

tom

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

[Vladimir Kozlov-5]

Tom Rodriguez wrote:
>> You use fetch_static_field() only to read Integer.sizeTable. Does it need
>> to be so generalized? But you can keep it as it is.
>
> Originally I was going to read some other fields so I needed something general.  It's based on do_get_xxx and I don't see any reason to simplify it.  I could move it over into GraphKit.
>

OK.

>> In int_stringSize(), I think, TypeAryPtr::INTS memory should be used instead of
>> TypeAryPtr::CHARS (for final_mem) and int_adr_idx (needs to add it) instead
>> of char_adr_idx.
>
> Actually there are no stores so it's not needed at all.  I'd added some debugging code that did a runtime call and needed the phi but I think I can remove it completely now.
>

OK.

>> Why 6 is limit for constant strings? Add some comments.
>
> Ok. It's just a number.  6 seems like an ok code space vs. speed tradeoff.

May be we should have it as flag or definition to be more visible?

Thanks,
Vladimir

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

[Tom Rodriguez]

On Nov 11, 2009, at 1:59 PM, Vladimir Kozlov wrote:

> Tom Rodriguez wrote:
>>> You use fetch_static_field() only to read Integer.sizeTable. Does it need
>>> to be so generalized? But you can keep it as it is.
>> Originally I was going to read some other fields so I needed something general.  It's based on do_get_xxx and I don't see any reason to simplify it.  I could move it over into GraphKit.
>
> OK.
>
>>> In int_stringSize(), I think, TypeAryPtr::INTS memory should be used instead of
>>> TypeAryPtr::CHARS (for final_mem) and int_adr_idx (needs to add it) instead
>>> of char_adr_idx.
>> Actually there are no stores so it's not needed at all.  I'd added some debugging code that did a runtime call and needed the phi but I think I can remove it completely now.
>
> OK.
>
>>> Why 6 is limit for constant strings? Add some comments.
>> Ok. It's just a number.  6 seems like an ok code space vs. speed tradeoff.
>
> May be we should have it as flag or definition to be more visible?

A flag seems excessive.  It's not like this is a critical tunable.  I can move it into an enum in PhaseStringOpts if you like.

  enum {
    // max length of constant string copy unrolling in copy_string                                                          
    unroll_string_copy_length = 6
  };

tom

>
> Thanks,
> Vladimir
>
>> tom
>>>
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> http://cr.openjdk.java.net/~never/6892658/