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

[Vladimir Ivanov]

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