On Fri, 27 Sep 2024 18:03:14 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.
>
> 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

Reply via email to