On Tue, 13 Aug 2024 01:39:33 GMT, Shaojin Wen <[email protected]> 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
