7179138: Incorrect result with String concatenation optimization
Summary: check for and skip diamond shaped NULL check code for the result of toString()
Reviewed-by: twisti, roland
--- a/hotspot/src/share/vm/opto/stringopts.cpp Fri Jun 22 10:40:48 2012 -0700
+++ b/hotspot/src/share/vm/opto/stringopts.cpp Tue Jun 26 09:06:16 2012 -0700
@@ -112,6 +112,7 @@
_arguments->ins_req(0, value);
_mode.insert_before(0, mode);
}
+
void push_string(Node* value) {
push(value, StringMode);
}
@@ -125,9 +126,56 @@
push(value, CharMode);
}
+ static bool is_SB_toString(Node* call) {
+ if (call->is_CallStaticJava()) {
+ CallStaticJavaNode* csj = call->as_CallStaticJava();
+ ciMethod* m = csj->method();
+ if (m != NULL &&
+ (m->intrinsic_id() == vmIntrinsics::_StringBuilder_toString ||
+ m->intrinsic_id() == vmIntrinsics::_StringBuffer_toString)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ static Node* skip_string_null_check(Node* value) {
+ // Look for a diamond shaped Null check of toString() result
+ // (could be code from String.valueOf()):
+ // (Proj == NULL) ? "null":"CastPP(Proj)#NotNULL
+ if (value->is_Phi()) {
+ int true_path = value->as_Phi()->is_diamond_phi();
+ if (true_path != 0) {
+ // phi->region->if_proj->ifnode->bool
+ BoolNode* b = value->in(0)->in(1)->in(0)->in(1)->as_Bool();
+ Node* cmp = b->in(1);
+ Node* v1 = cmp->in(1);
+ Node* v2 = cmp->in(2);
+ // Null check of the return of toString which can simply be skipped.
+ if (b->_test._test == BoolTest::ne &&
+ v2->bottom_type() == TypePtr::NULL_PTR &&
+ value->in(true_path)->Opcode() == Op_CastPP &&
+ value->in(true_path)->in(1) == v1 &&
+ v1->is_Proj() && is_SB_toString(v1->in(0))) {
+ return v1;
+ }
+ }
+ }
+ return value;
+ }
+
Node* argument(int i) {
return _arguments->in(i);
}
+ Node* argument_uncast(int i) {
+ Node* arg = argument(i);
+ int amode = mode(i);
+ if (amode == StringConcat::StringMode ||
+ amode == StringConcat::StringNullCheckMode) {
+ arg = skip_string_null_check(arg);
+ }
+ return arg;
+ }
void set_argument(int i, Node* value) {
_arguments->set_req(i, value);
}
@@ -206,9 +254,11 @@
void StringConcat::eliminate_unneeded_control() {
- eliminate_initialize(begin()->initialization());
for (uint i = 0; i < _control.size(); i++) {
Node* n = _control.at(i);
+ if (n->is_Allocate()) {
+ eliminate_initialize(n->as_Allocate()->initialization());
+ }
if (n->is_Call()) {
if (n != _end) {
eliminate_call(n->as_Call());
@@ -239,14 +289,15 @@
assert(result->_control.contains(other->_end), "what?");
assert(result->_control.contains(_begin), "what?");
for (int x = 0; x < num_arguments(); x++) {
- if (argument(x) == arg) {
+ Node* argx = argument_uncast(x);
+ if (argx == arg) {
// replace the toString result with the all the arguments that
// made up the other StringConcat
for (int y = 0; y < other->num_arguments(); y++) {
result->append(other->argument(y), other->mode(y));
}
} else {
- result->append(argument(x), mode(x));
+ result->append(argx, mode(x));
}
}
result->set_allocation(other->_begin);
@@ -327,14 +378,9 @@
while (worklist.size() > 0) {
Node* ctrl = worklist.pop();
- if (ctrl->is_CallStaticJava()) {
+ if (StringConcat::is_SB_toString(ctrl)) {
CallStaticJavaNode* csj = ctrl->as_CallStaticJava();
- ciMethod* m = csj->method();
- if (m != NULL &&
- (m->intrinsic_id() == vmIntrinsics::_StringBuffer_toString ||
- m->intrinsic_id() == vmIntrinsics::_StringBuilder_toString)) {
- string_calls.push(csj);
- }
+ string_calls.push(csj);
}
if (ctrl->in(0) != NULL && !_visited.test_set(ctrl->in(0)->_idx)) {
worklist.push(ctrl->in(0));
@@ -550,44 +596,40 @@
for (int c = 0; c < concats.length(); c++) {
StringConcat* sc = concats.at(c);
for (int i = 0; i < sc->num_arguments(); i++) {
- Node* arg = sc->argument(i);
- if (arg->is_Proj() && arg->in(0)->is_CallStaticJava()) {
+ Node* arg = sc->argument_uncast(i);
+ if (arg->is_Proj() && StringConcat::is_SB_toString(arg->in(0))) {
CallStaticJavaNode* csj = arg->in(0)->as_CallStaticJava();
- if (csj->method() != NULL &&
- (csj->method()->intrinsic_id() == vmIntrinsics::_StringBuilder_toString ||
- csj->method()->intrinsic_id() == vmIntrinsics::_StringBuffer_toString)) {
- for (int o = 0; o < concats.length(); o++) {
- if (c == o) continue;
- StringConcat* other = concats.at(o);
- if (other->end() == csj) {
+ for (int o = 0; o < concats.length(); o++) {
+ if (c == o) continue;
+ StringConcat* other = concats.at(o);
+ if (other->end() == csj) {
#ifndef PRODUCT
- if (PrintOptimizeStringConcat) {
- tty->print_cr("considering stacked concats");
- }
+ if (PrintOptimizeStringConcat) {
+ tty->print_cr("considering stacked concats");
+ }
#endif
- StringConcat* merged = sc->merge(other, arg);
- if (merged->validate_control_flow()) {
+ StringConcat* merged = sc->merge(other, arg);
+ if (merged->validate_control_flow()) {
#ifndef PRODUCT
- if (PrintOptimizeStringConcat) {
- tty->print_cr("stacking would succeed");
- }
+ if (PrintOptimizeStringConcat) {
+ tty->print_cr("stacking would succeed");
+ }
#endif
- if (c < o) {
- concats.remove_at(o);
- concats.at_put(c, merged);
- } else {
- concats.remove_at(c);
- concats.at_put(o, merged);
- }
- goto restart;
+ if (c < o) {
+ concats.remove_at(o);
+ concats.at_put(c, merged);
} else {
+ concats.remove_at(c);
+ concats.at_put(o, merged);
+ }
+ goto restart;
+ } else {
#ifndef PRODUCT
- if (PrintOptimizeStringConcat) {
- tty->print_cr("stacking would fail");
- }
+ if (PrintOptimizeStringConcat) {
+ tty->print_cr("stacking would fail");
+ }
#endif
- }
}
}
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/7179138/Test7179138_1.java Tue Jun 26 09:06:16 2012 -0700
@@ -0,0 +1,66 @@
+/*
+ * Copyright 2012 Skip Balk. All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 7179138
+ * @summary Incorrect result with String concatenation optimization
+ * @run main/othervm -Xbatch -XX:+IgnoreUnrecognizedVMOptions -XX:-TieredCompilation Test7179138_1
+ *
+ * @author Skip Balk
+ */
+
+public class Test7179138_1 {
+ public static void main(String[] args) throws Exception {
+ System.out.println("Java Version: " + System.getProperty("java.vm.version"));
+ long[] durations = new long[60];
+ for (int i = 0; i < 100000; i++) {
+ // this empty for-loop is required to reproduce this bug
+ for (long duration : durations) {
+ // do nothing
+ }
+ {
+ String s = "test";
+ int len = s.length();
+
+ s = new StringBuilder(String.valueOf(s)).append(s).toString();
+ len = len + len;
+
+ s = new StringBuilder(String.valueOf(s)).append(s).toString();
+ len = len + len;
+
+ s = new StringBuilder(String.valueOf(s)).append(s).toString();
+ len = len + len;
+
+ if (s.length() != len) {
+ System.out.println("Failed at iteration: " + i);
+ System.out.println("Length mismatch: " + s.length() + " <> " + len);
+ System.out.println("Expected: \"" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "\"");
+ System.out.println("Actual: \"" + s + "\"");
+ System.exit(97);
+ }
+ }
+ }
+ }
+}
+
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/7179138/Test7179138_2.java Tue Jun 26 09:06:16 2012 -0700
@@ -0,0 +1,66 @@
+/*
+ * Copyright 2012 Skip Balk. All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 7179138
+ * @summary Incorrect result with String concatenation optimization
+ * @run main/othervm -Xbatch -XX:+IgnoreUnrecognizedVMOptions -XX:-TieredCompilation Test7179138_2
+ *
+ * @author Skip Balk
+ */
+
+public class Test7179138_2 {
+ public static void main(String[] args) throws Exception {
+ System.out.println("Java Version: " + System.getProperty("java.vm.version"));
+ long[] durations = new long[60];
+ for (int i = 0; i < 100000; i++) {
+ // this empty for-loop is required to reproduce this bug
+ for (long duration : durations) {
+ // do nothing
+ }
+ {
+ String s = "test";
+ int len = s.length();
+
+ s = s + s;
+ len = len + len;
+
+ s = s + s;
+ len = len + len;
+
+ s = s + s;
+ len = len + len;
+
+ if (s.length() != len) {
+ System.out.println("Failed at iteration: " + i);
+ System.out.println("Length mismatch: " + s.length() + " <> " + len);
+ System.out.println("Expected: \"" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "\"");
+ System.out.println("Actual: \"" + s + "\"");
+ System.exit(0);
+ }
+ }
+ }
+ }
+}
+