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

Christian Thalinger christian.thalinger at oracle.com
Mon Jun 25 16:49:03 PDT 2012


Looks good.  I think it would be better to return v1 instead of assigning it to value here:

+           value = v1;

-- Chris

On Jun 25, 2012, at 4:19 PM, 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



More information about the hotspot-compiler-dev mailing list