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

Kris Mok rednaxelafx at gmail.com
Tue Jun 26 18:01:43 PDT 2012

I'm fine with that. Thank you, Vladimir.

On 2012-6-27, at 5:53, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

Thank you, Kris

Unfortunately I already pushed my fix. I filed RFE 7179968: improve String concatenation optimization There are other cases where we bailout and which could be fixed. We will combine those fixes. Thanks, Vladimir Krystal Mok wrote: Hi Vladimir, Sorry for being too late for this CR, but I'd like to suggest one further change in StringConcat::argumentuncast(): diff -r 751bd303aa45 src/share/vm/opto/stringopts.cpp --- a/src/share/vm/opto/stringopts.cpp Tue Jun 26 09:06:16 2012 -0700 +++ b/src/share/vm/opto/stringopts.cpp Wed Jun 27 03:50:45 2012 +0800 @@ -172,7 +172,7 @@ int amode = mode(i); if (amode == StringConcat::StringMode || amode == StringConcat::StringNullCheckMode) { - arg = skipstringnullcheck(arg); + arg = skipstringnullcheck(arg)->uncast(); } return arg; } This could help situations where there are extra levels of toString() calls in the way, e.g. public class MultipleUse { public static String foo() { String s = "testing"; s = new StringBuilder(s).append(s).toString().toString(); s = new StringBuilder(s).append(s).toString(); return s; } public static void main(String[] args) throws Exception { final String s = "testing"; for (int i = 0; i < 20000; i++) {_ _if (!(s + s + s + s).equals(foo())) {_ _System.out.println("bad string concat");_ _}_ _}_ _System.out.println("done");_ _System.in.read();_ _}_ _}_ _By adding the uncast() call, coalescing string concats would still work in this case._ _This should still be safe because argumentuncast() is only used when coalesing string concats, and that the result of a SB.toString() cannot be null._ _Regards,_ _Kris_ _On Tue, Jun 26, 2012 at 7:19 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote: http://cr.openjdk.java.net/~_kvn/7179138/webrev <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