Request for reviews (S): 7179138: Incorrect result with String concatenation optimization

[Vladimir Kozlov]

On 6/26/12 6:57 AM, Vladimir Ivanov wrote:
> Vladimir,
>
> The changes look good for me.
>
> I have a couple of small comments though:
>    1) Don't you want to add some assertions into skip_string_null_check
> to check the following precondition on value?
>   149         // phi->region->if_proj->ifnode->bool
>   150         BoolNode* b = value->in(0)->in(1)->in(0)->in(1)->as_Bool();

PhiNode::is_diamond_phi() checks for graph shape and as_Bool() has assert inside:

   type##Node *as_##type() const {                            \
     assert(is_##type(), "invalid node class");               \
     return (type##Node*)this;                                \
   }                                                          \

>
>    2) Why do you check only result string length in the tests and not
> it's content, since you know it? Moreover, "null" and "test" are both
> 4-char words :-)

The correct string is "testtest" vs "null" and doubles after each append. The failed (incorrect result) is 
"nullnulltesttest" instead of "testtesttesttesttesttesttesttest".

Vladimir

>
> Best regards,
> Vladimir Ivanov
>
> On 06/26/12 03:19, Vladimir Kozlov wrote:
>> http://cr.openjdk.java.net/~kvn/7179138/webrev
>>
>> 7179138: Incorrect result with String concatenation optimization
>>
>> The problem happens when one StringBuilder append/toString sequence uses
>> the result of previous StringBuilder sequence and both sequences are
>> merged by string concatenation optimization. Additional condition is the
>> usage of String.valueOf(s) call as argument of StringBuilder constructor
>> (which is implicitly generated by Eclipse java compiler).
>>
>> In normal case, when toString() result is directly referenced by the
>> constructor, string concatenation optimization will use input arguments
>> of previous StringBuilder append/toString sequence as additional
>> arguments of merged sequence instead of toString() result itself. It is
>> done because string concatenation optimization replaces the original
>> code with new one and it will throw away original calls, including
>> intermediate toString() calls.
>>
>> The problem with bug's case is toString() result is separated by Phi
>> node from diamond shaped code from valueOf(s) method (s==null ? "null" :
>> s) and it is kept as argument to new String concatenation code. But the
>> input to valueOf(s) check become dead after the call to toString() is
>> removed from graph. As result "null" string is used incorrectly instead.
>>
>> The fix is to look for diamond shaped code which checks for NULL the
>> result of toString() call and look through it as we do now for directly
>> referenced toString() results.
>>
>> I also fixed call nodes elimination code which did not remove Initialize
>> nodes from merged StringBuilder sequences. Currently it removes only
>> first one.
>>
>> Added regression tests from bug report.
>>
>> Tested with Hotspot regression tests, jdk java/lang tests, CTW.
>>
>> Thanks,
>> Vladimir