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

Reply via email to