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

Fri Dec 20 22:44:46 UTC 2024

On Thu, 19 Dec 2024 19:14:04 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'openjdk/master' into JDK-8321413
>  - Javadoc and some minor refactoring
>  - Move up Snippet setup as a builder
>  - Merge remote-tracking branch 'openjdk/master' into JDK-8321413
>  - Minor cleanup
>  - Rename based on feedback to emphasis building a snippet for loading a reference
>  - Fix typo and comment to match latest implementation
>  - Fix regression failed to setup helper methods properly
>  - Separate out ModuleDescriptorBuilder and use LoadableArray for paging
>  - Merge remote-tracking branch 'openjdk/master' into JDK-8321413
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/363ed696...d5a93295

Looks quite good.   I only skim on the javadoc/comment this time.   It seems okay for implementation use.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 96:

> 94: 
> 95:     /**
> 96:      * Describe a operand that can be load onto the operand stack.

Suggestion:

     * Describe an operand that can be load onto the operand stack.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 175:

> 173:     public static abstract class CollectionSnippetBuilder {
> 174:         /**
> 175:          * Tested page size of string array

Suggestion:

         * Default page size of string array

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 180:

> 178: 
> 179:         /**
> 180:          * Tested page size of enum array

Suggestion:

         * Default page size of enum array

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 190:

> 188: 
> 189:         /**
> 190:          * Arbitary default values based on 15K code size on ~30 bytes per element

Suggestion:

         * Default threshold based on 15K code size on ~30 bytes per element

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 227:

> 225:          */
> 226:         public CollectionSnippetBuilder enablePagination(String methodNamePrefix, int pageSize) {
> 227:             return enablePagination(methodNamePrefix, pageSize, Math.min(pageSize, DEFAULT_THRESHOLD));

shouldn't it use the current `activatePagingThreshold` value?  Otherwise, this method will override the threshold being set to a non-default value.

Same comment for `enablePagination(String methodNamePrefix)`

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 243:

> 241:         public CollectionSnippetBuilder disablePagination() {
> 242:             this.activatePagingThreshold = -1;
> 243:             this.methodNamePrefix = null;

suggest to reset `activatePagingThreshold` and `pageSize` to default value.   Set `methodNamePrefix` to null which indicates pagination is disabled.   So the constructor should initialize `methodNamePrefix` to null by default.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 408:

> 406:                             cob.invokestatic(
> 407:                                     ownerClassDesc,
> 408:                                     methodNamePrefix + (pageNo + 1),

Suggestion:

                                    methodNamePrefix + "_" + (pageNo + 1),

Suggest to add a character e.g. `_` or `$` to separate the prefix and counter since the prefix may end with a number which is hard to see which part is prefix and which is page number.

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

> 1043:          * the bytecode to populate that cache.
> 1044:          */
> 1045:         static record DedupSet(Map<Set<String>, Snippet> stringSets,

Suggestion:

        static record DedupSnippets(Map<Set<String>, Snippet> stringSets,

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

> 1099:             /*
> 1100:              * Generate bytecode to load a set onto the operand stack.
> 1101:              * Use cache if the set is references more than once.

Suggestion:

             * Use cache if the set is referenced more than once.

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

> 1214:                 // and is load from the cache array with
> 1215:                 //   dedupSetValues[index]
> 1216:                 // Otherwise, LoadableSet will load the set onto the operand stack.

This comment needs update.  No more `LoadableSet`

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

> 1237: 
> 1238:                 /*
> 1239:                  * Facilitate the cache in the class. Return a snippet to populate the cache in <clinit>.

Suggestion:

                 * Returns a snippet that populates the cached values in <clinit>.

test/jdk/tools/jlink/JLink20000Packages.java line 109:

> 107:         JImageGenerator.getJLinkTask()
> 108:                 .modulePath(out)
> 109:                 .output(src.resolve("out-jlink"))

Nit: define a variable for the image output directory that can be used in line 114 as well.

test/jdk/tools/jlink/SnippetsTest.java line 64:

> 62:  */
> 63: public class SnippetsTest {
> 64:     private static final boolean WRITE_CLASS_FILE = Boolean.parseBoolean(System.getProperty("DumpArraySnippetsTestClasses", "true"));

I assume this is set to true for debug only.  Default should be false?

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

PR Review: https://git.openjdk.org/jdk/pull/21022#pullrequestreview-2518387440
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894464733
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894461694
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894462638
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894464117
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894469332
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894472499
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894473793
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894428976
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894429347
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894446871
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894449712
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894415894
PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1894416655