RFR: 8362394: C2: Repeated stacked string concatenation fails with "Hit MemLimit" and other resourcing errors [v2]
Roberto Castañeda Lozano
rcastanedalo at openjdk.org
Tue Aug 12 09:18:17 UTC 2025
On Tue, 12 Aug 2025 06:32:13 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:
>
> move check / tweak test
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:
<img width="1171" height="341" alt="image" src="https://github.com/user-attachments/assets/fe694671-505e-41b8-b52e-37677af174b1" />
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.
src/hotspot/share/opto/stringopts.cpp line 299:
> 297: assert(result->_control.contains(_begin), "what?");
> 298:
> 299: const int concat_argument_upper_bound = 100;
Thanks for lifting this to a named constant. For consistency with [the style guide](https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#naming), 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`.
test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java line 30:
> 28: * consume too many compilation resources.
> 29: * @requires vm.compiler2.enabled
> 30: * @run main/othervm -XX:-OptoScheduling compiler.stringopts.TestStackedConcatsMany
Please add a comment explaining why `-XX:-OptoScheduling` is used.
test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java line 42:
> 40: public static void main (String... args) {
> 41: new StringBuilder(); // Trigger loading of the StringBuilder class.
> 42: String s = f(); // warmup call
Why do you need this warm-up call? Isn't it enough with calling `f` once?
test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsMany.java line 50:
> 48: if (!(s.equals(z))) {
> 49: throw new RuntimeException("wrong result.");
> 50: }
I think using `jdk.test.lib.Asserts.assertEQ` is preferable here.
-------------
Changes requested by rcastanedalo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26685#pullrequestreview-3109179654
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2269065810
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2269067300
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2269072168
PR Review Comment: https://git.openjdk.org/jdk/pull/26685#discussion_r2269078568
More information about the hotspot-compiler-dev
mailing list