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

[Henry Jen]

On Fri, 20 Dec 2024 22:32:00 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> 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/3754332f...d5a93295
>
> 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)`

I think about this, and it depends on how you look at the API.

The idea is that those three parameters are essential to enable pagination from disabled state. A default value is used when user is to enable pagination not specifying an explicit value, so a clear default is adapted rather than lingering with current setting.

Otherwise, user can use property methods to change page size or threshold.

I like the idea of use methodNamePrefix as the enable/disable check. However,  trying to explain the value of page size and threshold is current value, which would be default value without setting before is a little bit more complex, more so if trying to explain choosing the smaller value of page size and default threshold where we honestly have no idea if user have set a specific value or not.

I switched default to disable in the latest commit. And if it helps, I can remove those two enablePagination methods using default values.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1895312446