RFR: 8336856: Optimize String Concat [v39]

[Claes Redestad]

On Thu, 8 Aug 2024 11:43:07 GMT, Shaojin Wen <duke at openjdk.org> wrote:

>> This PR implements the same algorithm as the current generateMHInlineCopy based on bytecode to improve startup performance.
>
> Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix comments

This is looking very good now, and I think we have something here that is good enough to replace the current default strategy across all arities.

Collected some nits, at least some of which should be fixed before approval. Thanks!

src/java.base/share/classes/java/lang/StringConcatHelper.java line 83:

> 81: 
> 82:         @ForceInline
> 83:         private final String concat(char value) {

These mostly help avoid a bit of class spinning on startup, right? I think it's fine to add these now, but we should perhaps consider ways to pre-generate concat shapes more deliberately (e.g. using `GenerateJLIClassesHelper/-Plugin` to pre-spin concat classes when jlinking an image) rather than manually stamping out code here. If small arity concats benefit in throughput tests from a subtly different code shape then that should be reflected in the code generator.

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1073:

> 1071:      * Bytecode strategy.
> 1072:      *
> 1073:      * <p>This strategy emits StringBuilder chains as similar as possible

Outdated comment

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1082:

> 1080: 
> 1081:         static final ClassDesc CD_StringConcatHelper = ClassDesc.ofDescriptor("Ljava/lang/StringConcatHelper;");
> 1082:         static final ClassDesc CD_DecimalDigits      = ClassDesc.ofDescriptor("Ljdk/internal/util/DecimalDigits;");

Unused

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1099:

> 1097:         static final MethodTypeDesc MTD_String_Object   = MethodTypeDesc.of(CD_String, CD_Object);
> 1098: 
> 1099:         static final MethodTypeDesc MTD_GET_BYTES        = MethodTypeDesc.of(CD_void, CD_Array_byte, CD_int, CD_byte);

Unused

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1424:

> 1422:                         bufSlot       = cb.allocateLocal(TypeKind.from(byte[].class)),
> 1423:                         constantsSlot = cb.allocateLocal(TypeKind.from(String[].class)),
> 1424:                         suffixSlot    = cb.allocateLocal(TypeKind.from(String.class));

Suggestion:

                    int lengthSlot    = cb.allocateLocal(TypeKind.IntType),
                        coderSlot     = cb.allocateLocal(TypeKind.ByteType),
                        bufSlot       = cb.allocateLocal(TypeKind.ReferenceType),
                        constantsSlot = cb.allocateLocal(TypeKind.ReferenceType),
                        suffixSlot    = cb.allocateLocal(TypeKind.ReferenceType);

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1463:

> 1461:                             if (maybeUTF16(cl)) {
> 1462:                                 if (cl == char.class) {
> 1463:                                     cb.loadLocal(TypeKind.from(cl), cb.parameterSlot(i));

Suggestion:

                                    cb.loadLocal(TypeKind.CharType, cb.parameterSlot(i));

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1530:

> 1528:                         if (needStringOf(cl)) {
> 1529:                             paramSlot = stringSlots[i];
> 1530:                             kind = TypeKind.from(String.class);

Suggestion:

                            kind = TypeKind.ReferenceType;

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1679:

> 1677:                             methodTypeDesc = PREPEND_char;
> 1678:                         } else {
> 1679:                             kind = TypeKind.from(String.class);

Suggestion:

                            kind = TypeKind.ReferenceType;

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

Changes requested by redestad (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20273#pullrequestreview-2233278967
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713928423
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713971752
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713973032
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713973228
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713959662
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713960801
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713962187
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1713963024