8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases #18953

cl4es · 2024-04-25T14:15:56Z

Splitting out the ASM-based version from #18690 to push that first under the JBS (to help backporting). Keeping #18690 open to rebase and follow-up on this as a subtask. See discussion in that #18690 for more details, discussion and motivation for this.

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-8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases (Bug - P4)

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to a4dfbfca
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 591d2d1c

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18953

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-25T14:17:11Z

👋 Welcome back redestad! 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 · 2024-04-25T14:17:22Z

@cl4es 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:

8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases

Reviewed-by: mchung, shade

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 15 new commits pushed to the master branch:

cfd19f0: 8329850: [AIX] Allow loading of different members of same shared library archive
ffd850f: 8309881: Qualified name of a type element depends on its origin (source vs class)
377f2e5: 8329862: libjli GetApplicationHome cleanups and enhance jli tracing
006f090: 8331150: RISC-V: Fix "bad AD file" bug
a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
b3bcc49: 8330181: Move PcDesc cache from nmethod header
1c238d4: 8319990: Update CLDR to Version 45.0
ce9eac3: 8331113: createJMHBundle.sh support configurable maven repo mirror
5af6b45: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
... and 5 more: https://git.openjdk.org/jdk/compare/b1b953c427b9562f050760ab3c2d0ae09aeec3e5...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 · 2024-04-25T14:18:09Z

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

core-libs

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 · 2024-04-25T14:20:57Z

Webrevs

liach

Only with ASM can we realize how concise ClassFile API is!

liach · 2024-04-25T16:46:25Z

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java

+     * to what javac would. No exact sizing of parameters or estimates.
+     */
+    private static final class SimpleStringBuilderStrategy {
+        static final int CLASSFILE_VERSION = ClassFile.latestMajorVersion();


Still breaks backward ASM port, we should use a fixed version like 52 for JAVA_8 and convert to latest only in the CF conversion later.

Good catch. In the code I am reviving this field was simply set to 52. Fixed.

mlchung

Nit: the copyright header needs update before integration.

liach · 2024-04-26T02:36:22Z

Also a side note for backport: ClassFileDumper exists only since JDK 21, so for earlier backports you need to use an alternative dumping method or remove dumping ability.

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java

cl4es · 2024-04-26T08:36:16Z

Also a side note for backport: ClassFileDumper exists only since JDK 21, so for earlier backports you need to use an alternative dumping method or remove dumping ability.

Yes, original code used ProxyClassDumper, which was renamed/reworked in 21 by JDK-8304846. I think we've gone as far as we can code-wise in ensuring this is cleanly backportable to at least 21. I'll add a comment about ProxyClassDumper.

cl4es · 2024-04-26T08:42:57Z

@shipilev are you OK with this?

shipilev

Looks okay, thanks. Only cosmetic nits:

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java

cl4es · 2024-04-26T09:38:13Z

Thanks, nits resolved. Also some minor improvement (improved for; use iconst(mv, len) as originally intended)

cl4es · 2024-04-26T09:51:49Z

Running another round of sanity testing (tier1+2) before integration.

cl4es · 2024-04-26T12:36:11Z

/integrate

openjdk · 2024-04-26T12:36:57Z

Going to push as commit 5e2ced4.
Since your change was applied there have been 17 commits pushed to the master branch:

2b7176a: 8330625: Compilation memory statistic: prevent tearing of the final report
e4ed7ce: 8331063: Some HttpClient tests don't report leaks
cfd19f0: 8329850: [AIX] Allow loading of different members of same shared library archive
ffd850f: 8309881: Qualified name of a type element depends on its origin (source vs class)
377f2e5: 8329862: libjli GetApplicationHome cleanups and enhance jli tracing
006f090: 8331150: RISC-V: Fix "bad AD file" bug
a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
b3bcc49: 8330181: Move PcDesc cache from nmethod header
1c238d4: 8319990: Update CLDR to Version 45.0
... and 7 more: https://git.openjdk.org/jdk/compare/b1b953c427b9562f050760ab3c2d0ae09aeec3e5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-26T12:37:03Z

@cl4es Pushed as commit 5e2ced4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

cl4es added 2 commits April 25, 2024 15:38

ASM fallback version

9b48f48

Adapt main PR feedback to ASM version

d8d5e5d

cl4es mentioned this pull request Apr 25, 2024

8331134: Port SimpleStringBuilderStrategy to use ClassFile API #18690

Closed

3 tasks

openjdk bot added the rfr Pull request is ready for review label Apr 25, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 25, 2024

liach reviewed Apr 25, 2024

View reviewed changes

Remove accidental use of java.lang.classfile

a4dfbfc

mlchung approved these changes Apr 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 26, 2024

mlchung reviewed Apr 26, 2024

View reviewed changes

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java Show resolved Hide resolved

Copyright and comment about ProxyClassDumper

591d2d1

shipilev approved these changes Apr 26, 2024

View reviewed changes

Fix nits, use iconst as intended

9b083f4

openjdk bot added the integrated Pull request has been integrated label Apr 26, 2024

openjdk bot closed this Apr 26, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 26, 2024

cl4es deleted the concat_sb_fallback_asm branch April 26, 2024 14:10

cl4es added a commit to cl4es/jdk that referenced this pull request Apr 26, 2024

Rebase after integration of openjdk#18953

f87fa1d

8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases #18953

8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases #18953

Uh oh!

Conversation

cl4es commented Apr 25, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 25, 2024

Uh oh!

openjdk bot commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 25, 2024

Uh oh!

mlbridge bot commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

liach Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

cl4es Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

liach commented Apr 26, 2024

Uh oh!

Uh oh!

cl4es commented Apr 26, 2024

Uh oh!

cl4es commented Apr 26, 2024

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cl4es commented Apr 26, 2024

Uh oh!

cl4es commented Apr 26, 2024

Uh oh!

cl4es commented Apr 26, 2024

Uh oh!

openjdk bot commented Apr 26, 2024

Uh oh!

openjdk bot commented Apr 26, 2024

Uh oh!

Uh oh!

cl4es commented Apr 25, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 25, 2024 •

edited

Loading

mlbridge bot commented Apr 25, 2024 •

edited

Loading