8341696: C2: Non-fluid StringBuilder pattern bails out in OptoStringConcat

Reviewed-by: epeter

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.
+// Fluent-chains are recognized by walking upwards along the receivers, starting from toString().
-// The criterion is that the length of argument is easy to work out in this phrase.
+// Once the allocation of the StringBuilder has been reached, DU pairs are examined to find the
-// It will drop complex cases such as Object.
+// 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).
+// PS: Only a certain subset of constructor and append methods are acceptable.
-// The append method is overlooked. It will fail at validate_control_flow() test.
+// 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()) {
-#ifndef PRODUCT
-        if (PrintOptimizeStringConcat) {
-          tty->print("giving up because the call is effectively dead");
-          cnode->jvms()->dump_spec(tty); tty->cr();
-        }
-#endif
-        break;
-      }
-      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 {
+      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 encountered unexpected signature ");
-        cnode->tf()->dump(); tty->cr();
+          cnode->tf()->dump();
+          tty->cr();
           cnode->in(TypeFunc::Parms + 1)->dump();
         }
 #endif
-      break;
+        return nullptr;
+      }
     }
   }
   return nullptr;

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;

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();
+    }
+
+}

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();
+    }
+}