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