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

[Christian Hagedorn]

On Thu, 21 Aug 2025 07:41:32 GMT, Daniel Skantz <dskantz at openjdk.org> wrote:

>> 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] https://github.com/openjdk/jdk/blob/0ceb366dc26e2e4f6252da9dd8930b016a5d46ba/src/hotspot/share/opto/stringopts.cpp#L303
>> 
>> [2] https://github.com/openjdk/jdk/blob/0ceb366dc26e2e4f6252da9dd8930b016a5d46ba/src/hotspot/share/opto/stringopts.cpp#L1806
>> 
>> 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.
>
> Daniel Skantz has updated the pull request incrementally with one additional commit since the last revision:
> 
>   compare order

The fix looks good to me, too!  A few small comments/suggestions.

src/hotspot/share/opto/stringopts.cpp line 56:

> 54:                                        // to restart at the initial JVMState.
> 55: 
> 56:   static constexpr uint STACKED_CONCAT_UPPER_BOUND = 256; // argument limit for a merged concat.

Can you add a comment how we ended up with 256?

src/hotspot/share/opto/stringopts.cpp line 319:

> 317:     // -- and bail out in that case.
> 318:     if (arguments_appended > STACKED_CONCAT_UPPER_BOUND) {
> 319:       return nullptr;

Should we also print an error message for `PrintOptimizeStringConcat` here?

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java line 26:

> 24: /*
> 25:  * @test
> 26:  * @bug 8357105

Wrong bug number:
Suggestion:

 * @bug 8362394

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java line 37:

> 35:  */
> 36: 
> 37: // The test uses -XX:-OptoScheduling to avoid the assert "too many D-U pinch points" on aarch64.

I assume this is due to JDK-8328078? Maybe you can also mention the bug number here for completeness.

-------------

PR Review: https://git.openjdk.org/jdk/pull/26685#pullrequestreview-3223638117
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2348385681
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2348379717
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2348366153
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2348364931