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
