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