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

[Vladimir Kozlov]

Thank you, Vladimir, for review.

Vladimir

Vladimir Ivanov wrote:
> Vladimir,
> 
> I see. Thanks for the clarifications!
> 
> Best regards,
> Vladimir Ivanov
> 
> On 06/26/12 19:01, Vladimir Kozlov wrote:
>> 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