8362394: C2: Repeated stacked string concatenation fails with "Hit MemLimit" and other resourcing errors #26685

danielogh · 2025-08-08T06:10:56Z

This PR addresses a bug in the stringopts phase. During string concatenation, repeated stacking of concatenations can lead to excessive compilation resource use and generation of questionable code as the merging of two StringBuilder-append-toString links sc1 and sc2 can result in a new StringBuilder with the size sc1->num_arguments() * sc2->num_arguments().

In the attached test, the size of the successively merged StringBuilder doubles on each merge -- there's 24 of them -- as the toString result of the first component is used twice in the second component [1], etc. Not only does the compiler hang on this test case, but the string concat optimization seems to give an arbitrary amount of back-to-back stores in the generated code depending on the number of stacked concatenations.

The proposed solution is to put an upper bound on the size of a merged concatenation, which guards against this case of repeated concatenations on the same string variable, and potentially other edge cases. 100 seems like a generous limit, and higher limits could be insufficient as each argument corresponds to about 20 new nodes later in replace_string_concat [2].

[1]

jdk/src/hotspot/share/opto/stringopts.cpp

Line 303 in 0ceb366

for (int y = 0; y < other->num_arguments(); y++) {

[2]

jdk/src/hotspot/share/opto/stringopts.cpp

Line 1806 in 0ceb366

for (int argi = 0; argi < sc->num_arguments(); argi++) {

Testing: T1-4.

Extra testing: verified that no method in T1-4 is being compiled with a merged concat candidate exceeding the suggested limit of 100 aguments, regardless of whether or not the later checks verify_control_flow() and verify_mem_flow pass.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8362394: C2: Repeated stacked string concatenation fails with "Hit MemLimit" and other resourcing errors (Bug - P3)

Reviewers

Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26685/head:pull/26685
$ git checkout pull/26685

Update a local copy of the PR:
$ git checkout pull/26685
$ git pull https://git.openjdk.org/jdk.git pull/26685/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26685

View PR using the GUI difftool:
$ git pr show -t 26685

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26685.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-08T06:11:54Z

👋 Welcome back dskantz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-08-08T06:12:10Z

@danielogh This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8362394: C2: Repeated stacked string concatenation fails with "Hit MemLimit" and other resourcing errors

Reviewed-by: rcastanedalo

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 345 new commits pushed to the master branch:

c74c60f: 8308094: Add a compilation timeout flag to catch long running compilations
78d50c0: 8358756: [s390x] Test StartupOutput.java crash due to CodeCache size
2e06a91: 8365841: RISC-V: Several IR verification tests fail after JDK-8350960 without Zvfh
... and 342 more: https://git.openjdk.org/jdk/compare/c68697e1786fac37402b729d05a47b2f6296a86c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-08-08T06:13:03Z

@danielogh The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-08-08T06:20:14Z

Webrevs

danielogh · 2025-08-11T06:04:32Z

src/hotspot/share/opto/stringopts.cpp

+            // sc->num_arguments() * other->num_arguments(),
+            // which is a problem in the case of repeated stacked concats.
+            // Put a limit at 100 arguments to guard against excessive resource use.
+            bool n_args_is_bounded = merged->num_arguments() < 100;


This check can be done in the merge method instead. It's also possible to increase the bound but it will need a live node check later in replace_string_concat.

robcasloz

Good catch, Daniel! I have a few comments, mostly about the test.

robcasloz · 2025-08-11T08:03:26Z

src/hotspot/share/opto/stringopts.cpp

+            // merge(other, arg) can return a concatenation of size
+            // sc->num_arguments() * other->num_arguments(),
+            // which is a problem in the case of repeated stacked concats.
+            // Put a limit at 100 arguments to guard against excessive resource use.


Could you extract this limit into a constant and give it a meaningful name?

robcasloz · 2025-08-11T08:05:58Z

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java

+        for (int i = 0; i < 10; i++) {
+            String s = f(" ");
+        }


This could be simplified into:

Suggested change

for (int i = 0; i < 10; i++) {

String s = f(" ");

}

new StringBuilder(); // Trigger loading of the StringBuilder class.

String s = f(" ");

Then you can narrow down the CompileOnly command to -XX:CompileOnly=compiler.stringopts.TestStackedConcatsMany::f.

It would be good to assert that f(" ") returns the expected result.

robcasloz · 2025-08-11T08:23:54Z

test/hotspot/jtreg/ProblemList.txt

Did you consider running the test with -XX:-OptoScheduling, as an alternative to problem listing? This should simply avoid the problematic assertion and would be more robust w.r.t. other platforms.

galderz · 2025-08-11T10:30:42Z

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java

+ * @test
+ * @bug 8357105
+ * @summary Test that repeated stacked string concatenations do not
+ *          consume too many compilation resources.


Is there a reasonable way to enhance the test to validate excessive resources? I'm not sure if the following example would work, but I'm wondering if there is something that can be measured deterministically. E.g. before with the given test there would be ~N IR nodes produced but now it would be a max of ~M, assuming that M is deterministically smaller than N.

There's a 80000 node limit by default and maybe the test could use a lower limit by specifying a value for the MaxNodeLimit flag. There is also the IR framework that can check for node counts for individual nodes.

Without the fix, the test currently gets a MemLimit assert in debug runs for consuming 1GB of memory as it is building up the _arguments arrays. The high number of IR nodes is created later in replace_string_concat if we get that far without timing out or reaching the memory limit.

I think the default node limit, memory limit and timeout for product runs may be enough to test the fix.

What do you think, @galderz ? Thanks!

Sorry I had forgotten to 👍 before, all good with your reply

robcasloz

I did a quick study of C2 compile time vs. arguments_appended by running TestStackedConcatsMany with different number of s = new StringBuilder().append(s).append(s).toString(); lines, and got the following results in my machine:

The plot suggests to me that the current limit of 100 could be relaxed without risking a resource usage explosion. 256 seems to me like a better balance in terms of enabling the optimization in more scenarios while still being far away from the explosion point.

I also have a fewer follow-up comments and requests, mostly about style.

robcasloz · 2025-08-12T08:14:48Z

src/hotspot/share/opto/stringopts.cpp

@@ -295,6 +295,9 @@ StringConcat* StringConcat::merge(StringConcat* other, Node* arg) {
  }
  assert(result->_control.contains(other->_end), "what?");
  assert(result->_control.contains(_begin), "what?");
+
+  const int concat_argument_upper_bound = 100;


Thanks for lifting this to a named constant. For consistency with the style guide, I think it would be better to use upper case or mixed case. Consider also making this constant a static member of StringConcat. Finally, you could also declare both the constant and arguments_appended as uint.

robcasloz · 2025-08-12T08:15:20Z

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java

+ * @summary Test that repeated stacked string concatenations do not
+ *          consume too many compilation resources.
+ * @requires vm.compiler2.enabled
+ * @run main/othervm -XX:-OptoScheduling compiler.stringopts.TestStackedConcatsMany


Please add a comment explaining why -XX:-OptoScheduling is used.

robcasloz · 2025-08-12T08:17:19Z

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java

+
+    public static void main (String... args) {
+        new StringBuilder(); // Trigger loading of the StringBuilder class.
+        String s = f(); // warmup call


Why do you need this warm-up call? Isn't it enough with calling f once?

robcasloz · 2025-08-12T08:19:46Z

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java

+        if (!(s.equals(z))) {
+            throw new RuntimeException("wrong result.");
+        }


I think using jdk.test.lib.Asserts.assertEQ is preferable here.

robcasloz

Thanks for addressing my comments, Daniel! Please re-test to ensure the new limit is OK on all Oracle's internal test configurations.

danielogh · 2025-08-18T13:31:27Z

Thanks @robcasloz for the review. Great to increase the upper bound with compilation time measurements. Tests still look good to me.

dean-long · 2025-08-21T00:29:36Z

src/hotspot/share/opto/stringopts.cpp

+    // Check if this concatenation would result in an excessive number of arguments
+    // -- leading to high memory use, compilation time, and later, a large number of IR nodes
+    // -- and bail out in that case.
+    if (STACKED_CONCAT_UPPER_BOUND < arguments_appended) {


Is it just me, or is it easier to read with the limit on the right?

danielogh · 2025-09-04T06:32:00Z

A comment to keep PR active.

danielogh added 2 commits August 6, 2025 13:31

fix

0cb47d6

PL

1299298

openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org rfr Pull request is ready for review labels Aug 8, 2025

danielogh commented Aug 11, 2025

View reviewed changes

robcasloz suggested changes Aug 11, 2025

View reviewed changes

galderz reviewed Aug 11, 2025

View reviewed changes

move check / tweak test

69596e6

robcasloz suggested changes Aug 12, 2025

View reviewed changes

danielogh added 2 commits August 12, 2025 10:48

changes

f379d16

comment

0535d1f

robcasloz approved these changes Aug 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 12, 2025

dean-long reviewed Aug 21, 2025

View reviewed changes

compare order

a3e4b06

openjdk bot removed the ready Pull request is ready to be integrated label Aug 21, 2025

robcasloz approved these changes Aug 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 21, 2025

8362394: C2: Repeated stacked string concatenation fails with "Hit MemLimit" and other resourcing errors #26685

Are you sure you want to change the base?

8362394: C2: Repeated stacked string concatenation fails with "Hit MemLimit" and other resourcing errors #26685

Conversation

danielogh commented Aug 8, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 8, 2025

Uh oh!

openjdk bot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 8, 2025

Uh oh!

mlbridge bot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robcasloz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robcasloz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robcasloz left a comment

Choose a reason for hiding this comment

Uh oh!

danielogh commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielogh commented Sep 4, 2025

Uh oh!

Uh oh!

danielogh commented Aug 8, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Aug 8, 2025 •

edited

Loading

mlbridge bot commented Aug 8, 2025 •

edited

Loading

danielogh commented Aug 18, 2025 •

edited

Loading