RFR: 8321413: IllegalArgumentException: Code length outside the allowed range while creating a jlink image [v3]

[Mandy Chung]

On Fri, 27 Sep 2024 18:03:14 GMT, Henry Jen <henryjen at 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.
>
> Henry Jen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Missing from last commit

Quick comments on this PR.

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

> 556:         private final int moduleDescriptorsPerMethod;
> 557: 
> 558:         private final ArrayList<Consumer<ClassBuilder>> amendaments = new ArrayList<>();

Typo: `s/amendaments/amendments/`

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

> 663:         private void genConstants(ClassBuilder clb) {
> 664:             var cinitSnippets = dedupSetBuilder.buildConstants(clb);
> 665:             if (!cinitSnippets.isEmpty()) {

Suggestion:

            var clinitSnippets = dedupSetBuilder.buildConstants(clb);
            if (!clinitSnippets.isEmpty()) {

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

> 793:                                    this.classDesc,
> 794:                                    helperMethodNamePrefix + "0",
> 795:                                    MethodTypeDesc.of(CD_void, CD_MODULE_DESCRIPTOR.arrayType())

The comment line 741-767 needs to be updated to reflect the new helper method as well as the new methods `module{$index}Packages` and `module{$index}Exports`

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

> 1712:             }
> 1713: 
> 1714:             class SetReference<T extends Comparable<T>> implements Comparable<SetReference<T>> {

The class name `SetReference` is not obvious what it does.   A comment would be helpful.

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

PR Review: https://git.openjdk.org/jdk/pull/21022#pullrequestreview-2341647851
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1783660750
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1783661198
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1783662487
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1783667229