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

Vladimir Kozlov vladimir.kozlov at oracle.com
Tue Jun 26 08:01:08 PDT 2012

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 skipstringnullcheck to check the following precondition on value? 149 // phi->region->ifproj->ifnode->bool 150 BoolNode* b = value->in(0)->in(1)->in(0)->in(1)->asBool();

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

More information about the hotspot-compiler-dev mailing list