RFR: 8336856: Efficient hidden class-based string concatenation strategy [v48]

[Claes Redestad]

On Tue, 13 Aug 2024 01:39:33 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:
> 
>   add jtreg HiddenClassUnloading

Thanks for adding the test! Looks pretty good, but could perhaps be simplified a bit and run fewer iterations with the same result. 

Marking it as `@require vm.flagless` might avoid uninteresting test failures on various exotic VM configurations that higher test tiers might otherwise try out. 

(Also found a few more suggestions to the code at large)

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

> 1224:                     cl = String.class;
> 1225:                 }
> 1226:                 paramTypes[i + 1] = ConstantUtils.classDesc(cl);

Suggestion:

                paramTypes[i + 1] = needString(cl) ? CD_String : ConstantUtils.classDesc(cl);

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

> 1267:                             clb.withSuperclass(CD_StringConcatBase)
> 1268:                                 .withFlags(ACC_FINAL | ACC_SUPER | ACC_SYNTHETIC)
> 1269:                                 .withMethodBody(INIT_NAME, MTD_INIT, ACC_PROTECTED, CONSTRUCTOR_BUILDER)

Suggestion:

                                .withMethodBody(INIT_NAME, MTD_INIT, 0, CONSTRUCTOR_BUILDER)

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

> 1411:                     int   paramCount  = concatArgs.parameterCount();
> 1412:                     int   thisSlot    = cb.receiverSlot();
> 1413:                     int[] stringSlots = new int[paramCount];

This array and the following loop strictly isn't needed: you can allocate the string slots just before astore, then derive those slots again in the two loops doing aload. They'll always start from the same slot and be in the same order.

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

> 1415:                         var cl = concatArgs.parameterType(i);
> 1416:                         if (needStringOf(cl)) {
> 1417:                             stringSlots[i] = cb.allocateLocal(TypeKind.from(String.class));

Suggestion:

                            stringSlots[i] = cb.allocateLocal(TypeKind.ReferenceType);

test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 32:

> 30:  * @test
> 31:  * @summary Test whether the hidden class unloading of StringConcatFactory works
> 32:  *

Suggestion:

 *
 * @requires vm.flagless


I suggest we add this (slightly controversial) flag which locks down so that testing won't test this over and over at higher tiers with all manner of VM flags that this test might fail at.

test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 67:

> 65:                     new Object[0]
> 66:             );
> 67:             MethodHandle mh = callSite.dynamicInvoker();

Actually invoking the concats seem unnecessary for this test; even with the rest of this method removed many thousands of classes is unloaded. We also seem to do pretty well with fewer iterations in the outer loop.

test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 89:

> 87: 
> 88:         long unloadedClassCount = ManagementFactory.getClassLoadingMXBean().getUnloadedClassCount();
> 89:         if (unloadedClassCount == 0) {

Sample `getUnloadedClassCount()` before going into the loop so that we check that there's progress.

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

PR Review: https://git.openjdk.org/jdk/pull/20273#pullrequestreview-2234482233
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714815159
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714657417
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714657078
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714654215
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714870451
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714854631
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714857823