diff --git a/src/hotspot/share/opto/stringopts.cpp b/src/hotspot/share/opto/stringopts.cpp index 793145e078d..f867fcf4d23 100644 --- a/src/hotspot/share/opto/stringopts.cpp +++ b/src/hotspot/share/opto/stringopts.cpp @@ -173,6 +173,9 @@ class StringConcat : public ResourceObj { assert(!_control.contains(ctrl), "only push once"); _control.push(ctrl); } + bool has_control(Node* ctrl) { + return _control.contains(ctrl); + } void add_constructor(Node* init) { assert(!_constructors.contains(init), "only push once"); _constructors.push(init); @@ -407,7 +410,66 @@ Node_List PhaseStringOpts::collect_toString_calls() { return string_calls; } -// Recognize a fluent-chain of StringBuilder/Buffer. They are either explicit usages +PhaseStringOpts::ProcessAppendResult PhaseStringOpts::process_append_candidate(CallStaticJavaNode* cnode, + StringConcat* sc, + ciMethod* m, + ciSymbol* string_sig, + ciSymbol* int_sig, + ciSymbol* char_sig) { + if (cnode->method() != nullptr && !cnode->method()->is_static() && + cnode->method()->holder() == m->holder() && + cnode->method()->name() == ciSymbols::append_name() && + (cnode->method()->signature()->as_symbol() == string_sig || + cnode->method()->signature()->as_symbol() == char_sig || + cnode->method()->signature()->as_symbol() == int_sig)) { + if (sc->has_control(cnode)) { + return ProcessAppendResult::AppendWasAdded; + } + sc->add_control(cnode); + Node* arg = cnode->in(TypeFunc::Parms + 1); + if (arg == nullptr || arg->is_top()) { +#ifndef PRODUCT + if (PrintOptimizeStringConcat) { + tty->print("giving up because the call is effectively dead"); + cnode->jvms()->dump_spec(tty); + tty->cr(); + } +#endif + return ProcessAppendResult::AbortOptimization; + } + + if (cnode->method()->signature()->as_symbol() == int_sig) { + sc->push_int(arg); + } else if (cnode->method()->signature()->as_symbol() == char_sig) { + sc->push_char(arg); + } else if (arg->is_Proj() && arg->in(0)->is_CallStaticJava()) { + CallStaticJavaNode* csj = arg->in(0)->as_CallStaticJava(); + if (csj->method() != nullptr && + csj->method()->intrinsic_id() == vmIntrinsics::_Integer_toString && + arg->outcnt() == 1) { + // _control is the list of StringBuilder calls nodes which + // will be replaced by new String code after this optimization. + // Integer::toString() call is not part of StringBuilder calls + // chain. It could be eliminated only if its result is used + // only by this SB calls chain. + // Another limitation: it should be used only once because + // it is unknown that it is used only by this SB calls chain + // until all related SB calls nodes are collected. + assert(arg->unique_out() == cnode, "sanity"); + sc->add_control(csj); + sc->push_int(csj->in(TypeFunc::Parms)); + } else { + sc->push_string(arg); + } + } else { + sc->push_string(arg); + } + return ProcessAppendResult::AppendWasAdded; + } + return ProcessAppendResult::CandidateIsNotAppend; +} + +// Recognize fluent-chain and non-fluent uses of StringBuilder/Buffer. They are either explicit usages // of them or the legacy bytecodes of string concatenation prior to JEP-280. eg. // // String result = new StringBuilder() @@ -416,18 +478,17 @@ Node_List PhaseStringOpts::collect_toString_calls() { // .append(123) // .toString(); // "foobar123" // -// PS: Only a certain subset of constructor and append methods are acceptable. -// The criterion is that the length of argument is easy to work out in this phrase. -// It will drop complex cases such as Object. +// Fluent-chains are recognized by walking upwards along the receivers, starting from toString(). +// Once the allocation of the StringBuilder has been reached, DU pairs are examined to find the +// constructor and non-fluent uses of the StringBuilder such as in this example: // -// Since it walks along the receivers of fluent-chain, it will give up if the codeshape is -// not "fluent" enough. eg. // StringBuilder sb = new StringBuilder(); // sb.append("foo"); // sb.toString(); // -// The receiver of toString method is the result of Allocation Node(CheckCastPP). -// The append method is overlooked. It will fail at validate_control_flow() test. +// PS: Only a certain subset of constructor and append methods are acceptable. +// The criterion is that the length of argument is easy to work out in this phrase. +// It will drop complex cases such as Object. // StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) { ciMethod* m = call->method(); @@ -466,7 +527,7 @@ StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) { if (cnode == nullptr) { alloc = recv->isa_Allocate(); if (alloc == nullptr) { - break; + return nullptr; } // Find the constructor call Node* result = alloc->result_cast(); @@ -478,7 +539,7 @@ StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) { alloc->jvms()->dump_spec(tty); tty->cr(); } #endif - break; + return nullptr; } Node* constructor = nullptr; for (SimpleDUIterator i(result); i.has_next(); i.next()) { @@ -489,6 +550,10 @@ StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) { use->method()->name() == ciSymbols::object_initializer_name() && use->method()->holder() == m->holder()) { // Matched the constructor. + if (constructor != nullptr) { + // The constructor again. We must only process it once. + continue; + } ciSymbol* sig = use->method()->signature()->as_symbol(); if (sig == ciSymbols::void_method_signature() || sig == ciSymbols::int_void_signature() || @@ -542,7 +607,16 @@ StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) { } #endif } - break; + } else if (use != nullptr) { + if (process_append_candidate(use, sc, m, string_sig, int_sig, char_sig) == ProcessAppendResult::AbortOptimization) { + // We must abort if process_append_candidate tells us to... + return nullptr; + } + // ...but we do not care if we really found an append or not: + // - If we found an append, that's perfect. Nothing further to do. + // - If this is a call to an unrelated method, validate_mem_flow() (and validate_control_flow()) + // will later check if this call prevents the optimization. So nothing to do here. + // We will continue to look for the constructor (if not found already) and appends. } } if (constructor == nullptr) { @@ -553,7 +627,7 @@ StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) { alloc->jvms()->dump_spec(tty); tty->cr(); } #endif - break; + return nullptr; } // Walked all the way back and found the constructor call so see @@ -568,62 +642,23 @@ StringConcat* PhaseStringOpts::build_candidate(CallStaticJavaNode* call) { } else { return nullptr; } - } else if (cnode->method() == nullptr) { - break; - } else if (!cnode->method()->is_static() && - cnode->method()->holder() == m->holder() && - cnode->method()->name() == ciSymbols::append_name() && - (cnode->method()->signature()->as_symbol() == string_sig || - cnode->method()->signature()->as_symbol() == char_sig || - cnode->method()->signature()->as_symbol() == int_sig)) { - sc->add_control(cnode); - Node* arg = cnode->in(TypeFunc::Parms + 1); - if (arg == nullptr || arg->is_top()) { + } else { + ProcessAppendResult result = process_append_candidate(cnode, sc, m, string_sig, int_sig, char_sig); + + if (result == ProcessAppendResult::AbortOptimization) { + return nullptr; + } else if (result == ProcessAppendResult::CandidateIsNotAppend) { + // some unhandled signature #ifndef PRODUCT if (PrintOptimizeStringConcat) { - tty->print("giving up because the call is effectively dead"); - cnode->jvms()->dump_spec(tty); tty->cr(); + tty->print("giving up because encountered unexpected signature "); + cnode->tf()->dump(); + tty->cr(); + cnode->in(TypeFunc::Parms + 1)->dump(); } #endif - break; + return nullptr; } - if (cnode->method()->signature()->as_symbol() == int_sig) { - sc->push_int(arg); - } else if (cnode->method()->signature()->as_symbol() == char_sig) { - sc->push_char(arg); - } else { - if (arg->is_Proj() && arg->in(0)->is_CallStaticJava()) { - CallStaticJavaNode* csj = arg->in(0)->as_CallStaticJava(); - if (csj->method() != nullptr && - csj->method()->intrinsic_id() == vmIntrinsics::_Integer_toString && - arg->outcnt() == 1) { - // _control is the list of StringBuilder calls nodes which - // will be replaced by new String code after this optimization. - // Integer::toString() call is not part of StringBuilder calls - // chain. It could be eliminated only if its result is used - // only by this SB calls chain. - // Another limitation: it should be used only once because - // it is unknown that it is used only by this SB calls chain - // until all related SB calls nodes are collected. - assert(arg->unique_out() == cnode, "sanity"); - sc->add_control(csj); - sc->push_int(csj->in(TypeFunc::Parms)); - continue; - } - } - sc->push_string(arg); - } - continue; - } else { - // some unhandled signature -#ifndef PRODUCT - if (PrintOptimizeStringConcat) { - tty->print("giving up because encountered unexpected signature "); - cnode->tf()->dump(); tty->cr(); - cnode->in(TypeFunc::Parms + 1)->dump(); - } -#endif - break; } } return nullptr; diff --git a/src/hotspot/share/opto/stringopts.hpp b/src/hotspot/share/opto/stringopts.hpp index 21be4109c7d..99d554838d7 100644 --- a/src/hotspot/share/opto/stringopts.hpp +++ b/src/hotspot/share/opto/stringopts.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2009, 2025, Oracle and/or its affiliates. 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 @@ -34,7 +34,7 @@ class IdealVariable; class PhaseStringOpts : public Phase { friend class StringConcat; - private: +private: PhaseGVN* _gvn; // List of dead nodes to clean up aggressively at the end @@ -53,6 +53,23 @@ class PhaseStringOpts : public Phase { // a single string construction. StringConcat* build_candidate(CallStaticJavaNode* call); + enum class ProcessAppendResult { + // Indicates that the candidate was indeed an append and process_append_candidate processed it + // accordingly (added it to the StringConcat etc.) + AppendWasAdded, + // The candidate turned out not to be an append call. process_append_candidate did not do anything. + CandidateIsNotAppend, + // The candidate is an append call, but circumstances completely preventing string concat + // optimization were detected and the optimization must abort. + AbortOptimization + }; + + // Called from build_candidate. Looks at an "append candidate", a call that might be a call + // to StringBuilder::append. If so, adds it to the StringConcat. + ProcessAppendResult process_append_candidate(CallStaticJavaNode* cnode, StringConcat* sc, + ciMethod* m, ciSymbol* string_sig, ciSymbol* int_sig, + ciSymbol* char_sig); + // Replace all the SB calls in concat with an optimization String allocation void replace_string_concat(StringConcat* concat); @@ -105,12 +122,13 @@ class PhaseStringOpts : public Phase { unroll_string_copy_length = 6 }; - public: +public: PhaseStringOpts(PhaseGVN* gvn); #ifndef PRODUCT static void print_statistics(); - private: + +private: static uint _stropts_replaced; static uint _stropts_merged; static uint _stropts_total; diff --git a/test/hotspot/jtreg/compiler/stringopts/TestFluidAndNonFluid.java b/test/hotspot/jtreg/compiler/stringopts/TestFluidAndNonFluid.java new file mode 100644 index 00000000000..fc4ffaddb86 --- /dev/null +++ b/test/hotspot/jtreg/compiler/stringopts/TestFluidAndNonFluid.java @@ -0,0 +1,134 @@ +/* + * Copyright (c) 2025 Oracle and/or its affiliates. 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 8341696 + * @summary Allow C2 to also optimize non-fluid string builder calls. + * @library /test/lib / + * @run driver compiler.c2.irTests.stringopts.TestFluidAndNonFluid + */ +package compiler.c2.irTests.stringopts; + +import compiler.lib.ir_framework.*; +import jdk.test.lib.Asserts; + +public class TestFluidAndNonFluid { + + public static int unknown = 1; + + public static void main(String[] args) { + // Dont inline any StringBuilder methods for this IR test to check if string opts are applied or not. + TestFramework.runWithFlags("-XX:CompileCommand=dontinline,java.lang.StringBuilder::*"); + } + + @DontInline + public static void opaque(StringBuilder builder) { + builder.append("Z"); + } + + @Run(test = {"fluid", "nonFluid", "nonFinal", "nonFluidExtraneousVariable", "nonFluidConditional", + "nonFluidOpaqueCall"}) + public void runMethod() { + Asserts.assertEQ("0ac", fluidNoParam()); + Asserts.assertEQ("ac", nonFluidNoParam()); + Asserts.assertEQ("ac", fluid("c")); + Asserts.assertEQ("ac", nonFluid("c")); + Asserts.assertEQ("ac", nonFinal("c")); + Asserts.assertEQ("ac", nonFluidExtraneousVariable("c")); + Asserts.assertEQ("ac", nonFluidConditional("c")); + Asserts.assertEQ("aZ", nonFluidOpaqueCall()); + } + + @Test + @IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString", IRNode.INTRINSIC_TRAP}) + public static String fluidNoParam() { + return new StringBuilder("0").append("a").append("c").toString(); + } + + @Test + @IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString", IRNode.INTRINSIC_TRAP}) + public static String nonFluidNoParam() { + final StringBuilder sb = new StringBuilder(); + sb.append("a"); + sb.append("c"); + return sb.toString(); + } + + @Test + @IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"}) + public static String fluid(String a) { + return new StringBuilder().append("a").append(a).toString(); + } + + @Test + @IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"}) + public static String nonFluid(String a) { + final StringBuilder sb = new StringBuilder(); + sb.append("a"); + sb.append(a); + return sb.toString(); + } + + @Test + @IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"}) + public static String nonFinal(String a) { + StringBuilder sb = new StringBuilder(); + sb.append("a"); + sb.append(a); + return sb.toString(); + } + + @Test + @IR(failOn = {IRNode.ALLOC_OF, "StringBuilder", IRNode.CALL_OF_METHOD, "toString"}) + public static String nonFluidExtraneousVariable(String a) { + final StringBuilder sb = new StringBuilder(); + final StringBuilder x = sb; + sb.append("a"); + x.append(a); + return sb.toString(); + } + + @Test + @IR(counts = {IRNode.ALLOC_OF, "StringBuilder", "1", IRNode.CALL_OF_METHOD, "toString", "1"}) + @IR(failOn = IRNode.INTRINSIC_TRAP) + static String nonFluidConditional(String a) { + final StringBuilder sb = new StringBuilder(); + sb.append("a"); + if (unknown == 1) { + sb.append(a); + } + return sb.toString(); + } + + @Test + @IR(counts = {IRNode.ALLOC_OF, "StringBuilder", "1", IRNode.CALL_OF_METHOD, "toString", "1"}) + @IR(failOn = IRNode.INTRINSIC_TRAP) + static String nonFluidOpaqueCall() { + final StringBuilder sb = new StringBuilder(); + sb.append("a"); + opaque(sb); + return sb.toString(); + } + +} diff --git a/test/micro/org/openjdk/bench/vm/compiler/FluidSBBench.java b/test/micro/org/openjdk/bench/vm/compiler/FluidSBBench.java new file mode 100644 index 00000000000..794ff768678 --- /dev/null +++ b/test/micro/org/openjdk/bench/vm/compiler/FluidSBBench.java @@ -0,0 +1,61 @@ +/* + * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. + * Copyright (c) 2025, Oracle and/or its affiliates. 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. + * + */ + +package org.openjdk.bench.vm.compiler; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Scope; +import java.util.concurrent.TimeUnit; + +@Warmup(iterations = 3, time = 300, timeUnit = TimeUnit.MILLISECONDS) +@Measurement(iterations = 3, time = 300, timeUnit = TimeUnit.MILLISECONDS) +@Fork(value = 1, jvmArgsAppend = {"-XX:+UseParallelGC", "-Xmx1g", "-Xms1g"}) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class FluidSBBench { + static final String PREFIX = "a"; + String foo = "aaaaa aaaaa aaaaa aaaaa aaaaa"; + + @Benchmark + public String fluid() { + return new StringBuilder().append(PREFIX).append(foo).toString(); + } + + @Benchmark + public String nonFluid() { + final StringBuilder sb = new StringBuilder(); + sb.append(PREFIX); + sb.append(foo); + return sb.toString(); + } +}