On Tue, 4 Jul 2023 11:00:13 GMT, Oliver Kopp <d...@openjdk.org> wrote:

>> Fix for [JDK-8240567](https://bugs.openjdk.org/browse/JDK-8240567): 
>> "MethodTooLargeException thrown while creating a jlink image".
>> 
>> Java still has a 64kb limit: A method may not be longer than 64kb. The idea 
>> of the fix is to split up the generated methods in several smaller methods
>> 
>> This is a follow-up to https://github.com/openjdk/jdk/pull/10704. GitHub did 
>> not allow me to re-open the PR, because I did a force-push to have one 
>> commit.
>
> Oliver Kopp has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refine test to fill DedupSetBuilder

It's looking pretty good. 

About the test, I don't see `ArrayList::add` in the generated bytecode of 
`sub2-13`.   The dedup string set is used for the targets of qualified exports 
and opens and uses.   The modifiers set of `requires` is deduplicated but not 
the required module names.

I assume you want this be backported.  If so, we should give it some baked time 
after it's integrated to the main line.    I'm okay if you want to extend the 
test in a follow up.     You can extract the generated class to verify:

$ jimage extract --dir=dir out-jlink/lib/modules
$ javap -p -v dir/java.base/jdk/internal/module/SystemModules$all.class

test/jdk/tools/jlink/JLink100Modules.java line 118:

> 116:         Path bin = binDir.resolve("java");
> 117: 
> 118:         ProcessBuilder processBuilder = new 
> ProcessBuilder(bin.toString(), "-XX:+UnlockDiagnosticVMOptions", 
> "-XX:+BytecodeVerificationLocal", "-m", 
> "bug8240567x/testpackage.JLink100ModulesTest");

please wrap this long line into multiple lines.

test/jdk/tools/jlink/JLink100Modules.java line 123:

> 121:         Process process = processBuilder.start();
> 122:         int exitCode = process.waitFor();
> 123:         if (exitCode != 0) throw new AssertionError("Exit code is not 
> 0");

Suggestion:

        if (exitCode != 0)
             throw new AssertionError("JLink100ModulesTest failed to launch");

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

PR Review: https://git.openjdk.org/jdk/pull/14408#pullrequestreview-1514989193
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1253405169
PR Review Comment: https://git.openjdk.org/jdk/pull/14408#discussion_r1253405958

Reply via email to