Request for reviews (S): 7179138: Incorrect result with String concatenation optimization
Krystal Mok
rednaxelafx at gmail.com
Tue Jun 26 13:06:57 PDT 2012
Hi Vladimir,
Sorry for being too late for this CR, but I'd like to suggest one further
change in StringConcat::argument_uncast():
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 = skip_string_null_check(arg);
+ arg = skip_string_null_check(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 argument_uncast() 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
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120627/42aaea53/attachment.html
More information about the hotspot-compiler-dev
mailing list