On Mon, 16 Sep 2024 17:50:52 GMT, Henry Jen <henry...@openjdk.org> wrote:
> This PR split out large array/set construction into separate factory methods > to avoid oversized method trying to construct several of those. > > In order to do that, we will need to generate those help methods on demand in > the class builder. Here we have two approach, one is for dedup set, which is > processed in advance so we can know what methods should be created. > > Another is for random set, such as packages, thus we put those request into a > queue to amend the class later. > > To keep the optimization of caching built value that are references more than > once, it was implemented using local vars, which doesn't work well for helper > methods. The existing approach to populate local vars doesn't work well with > larger scope of split operation, as the slot was allocated on lazily built, > but the transfer is captured in advance, this count could mismatch as built > time and run time. > > So we make this build in advance, and use a static array for values referred > more than once. > > All the codegen instead of giving index to be loaded, the builder snippet now > load the wanted set/array to the operand stack to be consistent. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 669: > 667: CLASS_INIT_NAME, > 668: MTD_void, > 669: ACC_PUBLIC | ACC_STATIC, Suggestion: ACC_STATIC, By convention, we simply set the static flag without anything else set. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1248: > 1246: methodName, > 1247: MethodTypeDesc.of(CD_EXPORTS.arrayType()), > 1248: ACC_PRIVATE | ACC_FINAL | ACC_STATIC, Just curious, why do we mark our split provider methods final? The final flag is only used for hiding. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1427: > 1425: var methodName = "module" + index + "Packages"; > 1426: addModuleHelpers(clb -> { > 1427: clb.withMethodBody( We should indent this lambda. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1598: > 1596: > 1597: int counterStoredValues = 0; > 1598: ClassDesc owner; Suggestion: final ClassDesc owner; src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 1804: > 1802: > 1803: static <T extends Enum<T>> BiConsumer<CodeBuilder, T> > getEnumLoader(ClassDesc enumClassDesc) { > 1804: return (cob, element) -> cob.getstatic(enumClassDesc, > element.toString(), enumClassDesc); Suggestion: return (cob, element) -> cob.getstatic(enumClassDesc, element.name(), enumClassDesc); test/jdk/tools/jlink/JLink3500Packages.java line 2: > 1: /* > 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. Suggestion: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. test/jdk/tools/jlink/JLink3500Packages.java line 35: > 33: /* > 34: * @test > 35: * @summary Make sure that 4000 packages in a uber jar can be linked > using jlink. Should we mention that the number 3500 or 4000 is an arbitrary large number greater than `ModuleDescriptorBuilder.SET_SIZE_THRESHOLD` to trigger the provider method generation? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1776117490 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1776117924 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1776110978 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1776113761 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1776114256 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1776123965 PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1776108517