On Tue, 13 Aug 2024 01:39:33 GMT, Shaojin Wen <d...@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

Reply via email to