On Fri, 13 Dec 2024 19:30:19 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:
>
> Move up Snippet setup as a builder
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
line 1113:
> 1111: Map<Set<String>, Snippet> snippets = new
> HashMap<>(map.size());
> 1112: map.entrySet().forEach(entry ->
> snippets.put(entry.getKey(),
> 1113: buildStringSet(clb, entry.getValue())));
Nit:
Suggestion:
map.entrySet().forEach(entry -> snippets.put(entry.getKey(),
buildStringSet(clb, entry.getValue())));
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
line 1120:
> 1118: Map<Set<T>, Snippet> snippets = new
> HashMap<>(map.size());
> 1119: map.entrySet().forEach(entry ->
> snippets.put(entry.getKey(),
> 1120: buildEnumSet(clb, entry.getValue())));
Suggestion:
map.entrySet().forEach(entry -> snippets.put(entry.getKey(),
buildEnumSet(clb,
entry.getValue())));
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
line 1136:
> 1134: /*
> 1135: * SetReference count references to the set, and use
> LoadableSet under the hood to
> 1136: * support paginiation as needed.
Suggestion:
* support pagination as needed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1884632423
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1884632724
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1884633111
