stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

7179138: Incorrect result with String concatenation optimization

Browse Source 
Check for and skip diamond shaped NULL check code for the result of toString()

Reviewed-by: twisti, roland
This commit is contained in:
Vladimir Kozlov 2012-06-26 09:06:16 -07:00
parent 6bc673930f
commit 8c4cefaa96
3 changed files with 216 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

126
hotspot/src/share/vm/opto/stringopts.cpp
View File

@ -112,6 +112,7 @@ class StringConcat : public ResourceObj {
_arguments->ins_req(0, value);
_mode.insert_before(0, mode);
}
void push_string(Node* value) {
push(value, StringMode);
}
@ -125,9 +126,56 @@ class StringConcat : public ResourceObj {
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 @@ class StringConcat : public ResourceObj {
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 @@ StringConcat* StringConcat::merge(StringConcat* other, Node* arg) {
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 @@ Node_List PhaseStringOpts::collect_toString_calls() {
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 @@ PhaseStringOpts::PhaseStringOpts(PhaseGVN* gvn, Unique_Node_List*):
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");
}
#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;
} else {
#ifndef PRODUCT
if (PrintOptimizeStringConcat) {
tty->print_cr("stacking would fail");
}
#endif
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;
} else {
#ifndef PRODUCT
if (PrintOptimizeStringConcat) {
tty->print_cr("stacking would fail");
}
#endif
}
}
}

66
hotspot/test/compiler/7179138/Test7179138_1.java Normal file
View File

@ -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);
}
}
}
}
}

66
hotspot/test/compiler/7179138/Test7179138_2.java Normal file
View File

@ -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);
}
}
}
}
}