Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote: >> 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. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidental use of java.lang.classfile @shipilev are you OK with this? - PR Comment: https://git.openjdk.org/jdk/pull/18953#issuecomment-2078922183
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Fri, 26 Apr 2024 02:36:22 GMT, Chen Liang wrote: > 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](https://bugs.openjdk.org/browse/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. - PR Comment: https://git.openjdk.org/jdk/pull/18953#issuecomment-2078910803
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Fri, 26 Apr 2024 03:14:54 GMT, Mandy Chung wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove accidental use of java.lang.classfile
>
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line
> 1057:
>
>> 1055: */
>> 1056: private static final class SimpleStringBuilderStrategy {
>> 1057: static final int CLASSFILE_VERSION = 52; // JDK 8
>
> Alternatively, this can use `ClassFileFormatVersion.latest().major()` which
> exists since JDK 20.
As the original code just had 52 verbatim and I don't know how far we'll want
to backport this I think it's fine to leave this at 52.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580682911
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote:
>> 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.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove accidental use of java.lang.classfile
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1057:
> 1055: */
> 1056: private static final class SimpleStringBuilderStrategy {
> 1057: static final int CLASSFILE_VERSION = 52; // JDK 8
Alternatively, this can use `ClassFileFormatVersion.latest().major()` which
exists since JDK 20.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580432036
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote: >> 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. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidental use of java.lang.classfile 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. - PR Comment: https://git.openjdk.org/jdk/pull/18953#issuecomment-2078532971
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 19:53:46 GMT, Claes Redestad wrote: >> 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. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidental use of java.lang.classfile Nit: the copyright header needs update before integration. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18953#pullrequestreview-2023964506
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Thu, 25 Apr 2024 16:46:25 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove accidental use of java.lang.classfile
>
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line
> 1059:
>
>> 1057: */
>> 1058: private static final class SimpleStringBuilderStrategy {
>> 1059: 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](https://github.com/cl4es/jdk/commit/36c4b11bc6cf5a008d5935934aa75f2d2bbe6a23#diff-1339c269a3729d849799d29a7431ccd508a034ced91c1796b952795396843891L771)
this field was simply set to `52`. Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580047931
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
> 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. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove accidental use of java.lang.classfile - Changes: - all: https://git.openjdk.org/jdk/pull/18953/files - new: https://git.openjdk.org/jdk/pull/18953/files/d8d5e5d3..a4dfbfca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18953=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18953=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18953.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18953/head:pull/18953 PR: https://git.openjdk.org/jdk/pull/18953
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Fri, 12 Apr 2024 10:19:27 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line
>> 1430:
>>
>>> 1428: cb.new_(STRING_BUILDER);
>>> 1429: cb.dup();
>>> 1430: cb.invokespecial(STRING_BUILDER, "",
>>> MethodTypeDesc.ofDescriptor("()V"));
>>
>> Would there be value in initializing to a larger capacity? Given the number
>> of append calls, seems the default cap of 16 is unlikely to be sufficient.
>
> Possibly. I tried a few simple variants that initialized the `StringBuilder`
> capacity at various guesses, such as sum of constant sizes + some factor
> based on args, but across a diverse set of micros that gives both some wins
> and some regressions. Getting the estimation just right is pretty tricky,
> especially when size of the arguments are arbitrary (like for any
> String/Object arg).
What are the scenarios which had regressions?
Given the conservative growth for StringBuilder, it surprises me a bit that any
scenario would regress.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1564165946
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
> This patch suggests a workaround to an issue with huge SCF MH expression > trees taking excessive JIT compilation resources by reviving (part of) the > simple bytecode-generating strategy that was originally available as an > all-or-nothing strategy choice. > > Instead of reintroducing a binary strategy choice I propose a threshold > parameter, controlled by > `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions > below or at this threshold there's no change, for expressions with an arity > above it we use the `StringBuilder`-chain bytecode generator. > > There are a few trade-offs at play here which influence the choice of > threshold. The simple high arity strategy will for example not see any reuse > of LambdaForms but strictly always generate a class per indy callsite, which > means we might end up with a higher total number of classes generated and > loaded in applications if we set this value too low. It may also produce > worse performance on average. On the other hand there is the observed > increase in C2 resource usage as expressions grow unwieldy. On the other > other hand high arity expressions are likely rare to begin with, with less > opportunities for sharing than the more common low-arity expressions. > > I turned the submitted test case into a few JMH benchmarks and did some > experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: > > Baseline strategy: > 13 args: 6.3M > 23 args: 18M > 123 args: 868M > > `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: > 13 args: 2.11M > 23 args: 3.67M > 123 args: 4.75M > > For 123 args the memory overhead of the baseline strategy is 180x, but for 23 > args we're down to a 5x memory overhead, and down to a 3x overhead for 13 > args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) > I've conservatively chosen a threshold at arity 20. This keeps C2 resource > pressure at a reasonable level (< 18M) while avoiding perturbing performance > at the vast majority of call sites. > > I was asked to use the new class file API for mainline. There's a version of > this patch implemented using ASM in 7c52a9f which might be a reasonable basis > for a backport. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: @liach feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18690/files - new: https://git.openjdk.org/jdk/pull/18690/files/b5928994..1c3d354c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18690=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18690=00-01 Stats: 32 lines in 1 file changed: 4 ins; 23 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18690/head:pull/18690 PR: https://git.openjdk.org/jdk/pull/18690
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v2]
On Fri, 12 Apr 2024 14:26:04 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> @liach feedback
>
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line
> 1406:
>
>> 1404: @Override
>> 1405: public void accept(ClassBuilder clb) {
>> 1406: clb.withFlags(AccessFlag.PUBLIC,
>> AccessFlag.FINAL, AccessFlag.SUPER, AccessFlag.SYNTHETIC)
>
> Why is this hidden class public?
Removed `PUBLIC`.
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line
> 1458:
>
>> 1456:
>> 1457: // Load the argument of type cl at slot onto stack, return the
>> number of argument stack slots consumed.
>> 1458: private static int load(CodeBuilder cb, Class cl, int slot)
>> {
>
> You can replace all calls to `load(cb, cl, slot)` with
> `cb.loadInstruction(TypeKind.from(cl), slot)`, and the retrn slot count can
> be accessed by `TypeKind::slotSize`
Thanks, I've tried to simplify.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1562702306
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1562701048
