RFR: 8240567: MethodTooLargeException thrown while creating a jlink image [v6]

[Mandy Chung]

On Thu, 29 Jun 2023 19:15:15 GMT, Oliver Kopp <duke at openjdk.org> wrote:

>> Fix for [JDK-8240567](https://bugs.openjdk.org/browse/JDK-8240567): "MethodTooLargeException thrown while creating a jlink image".
>> 
>> Java still has a 64kb limit: A method may not be longer than 64kb. The idea of the fix is to split up the generated methods in several smaller methods
>> 
>> This is a follow-up to https://github.com/openjdk/jdk/pull/10704. GitHub did not allow me to re-open the PR, because I did a force-push to have one commit.
>
> Oliver Kopp has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix threshold

Thanks for fixing this.   The approach looks okay.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 669:

> 667:          */
> 668:         private void genModuleDescriptorsMethod(ClassBuilder clb) {
> 669:             if (moduleInfos.size() <= 75) {

The plugin can take an optional argument to configure the max number of module infos to split in case it hits the limit in a future case.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 683:

> 681:                                         minfo.descriptor(),
> 682:                                         minfo.packages(),
> 683:                                         index).build();

Formatting nit: align the parameters with the first one as the original code does.  Same applies to line 751-753

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 691:

> 689:             }
> 690: 
> 691:             List<List<ModuleInfo>> splitModuleInfos = new ArrayList<>();

Please add the comments to describe what this does.   Pseudo code may also help.   It'd be helpful to explain the reason why the Sub method stores and restores what local variables.

I also wonder if you consider `moduleDescriptors` does this:

    ArrayList<Object> localVars = new ArrayList<>();
    moduleDescriptorsSub0(moduleInfos, localVars);
    .... // add new local variables to localVars
    moduleDescriptorsSub1(moduleInfos, localVars);
    .... // add new local variables to localVars
    :
    :


The same `localVars` array list is used and just add the new local variables defined from each batch (no need to create a new ArrayList and stores local variables every time)

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 701:

> 699:             }
> 700: 
> 701:             final String helperMethodNamePrefix = "moduleDescriptorsSub";

Perhaps a shorter name.   Also, prefer to drop the final modifier from these local variables (which adds noise).

Suggestion:

            String helperMethodNamePrefix = "sub";

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 731:

> 729: 
> 730:             final int[] globalCount = {0};
> 731:             for (final int[] index = {0}; index[0] < splitModuleInfos.size(); index[0]++) {

Suggestion:

            for (int n = 0, count = 0; n < splitModuleInfos.size(); count += splitModuleInfos.get(n).size(), n++) {
                int index = n;          // the index of which ModuleInfo being processed in the current batch
                int start = count;   // the start index to the return ModuleDescriptor array for the current batch


I prefer to avoid using the array to workaround the final variable used within lambda.   Free feel to rename these variables and comment to make it clear.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 737:

> 735:                         ACC_PUBLIC,
> 736:                         cob -> {
> 737:                             List<ModuleInfo> moduleInfosPackage = splitModuleInfos.get(index[0]);

It's not a package.   Maybe `currentBatch`?

Suggestion:

                            List<ModuleInfo> currentBatch = splitModuleInfos.get(index[0]);

test/jdk/tools/jlink/JLink100Modules.java line 40:

> 38:  * @library ../lib
> 39:  * @modules java.base/jdk.internal.jimage
> 40:  *          jdk.jdeps/com.sun.tools.classfile

I guess you copied this from other jlink tests.   Do you know why jlink tests need `com.sun.tools.classfile`?

test/jdk/tools/jlink/JLink100Modules.java line 47:

> 45:  *          jdk.compiler
> 46:  * @build tests.*
> 47:  * @run main/othervm -verbose:gc -Xmx1g -Xlog:init=debug -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal JLink100Modules

This patch does not change the bytecode if it's less than 75 modules.  You want to apply `-XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal`  flags also to a custom image with the new bytecodes.   So this test should include an execution of `JLink100ModulesTest`  from `out-jlink` image with these flags.

-------------

PR Review: https://git.openjdk.org/jdk/pull/14408#pullrequestreview-1506071351
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247152818
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247153724
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247170257
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247178553
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247176136
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247176887
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247137347
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1247143921