RFR: 8339332: Clean up the java launcher code

[David Holmes]

On Tue, 10 Sep 2024 06:15:57 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review of this change which cleans up the code in the `java` launcher? The original motivation for the change was to prevent the `java` launcher's C code from parsing a jar's manifest when launching an application through `java -jar foo.jar`. The jar parsing code in C currently doesn't have the same level of robustness and features as what's available in the Java side implementation of `Zip/JarFile` API in `java.util.zip`. Among them, one is the lack of ZIP64 support in the launcher's jar parsing code. That issue was noticed in https://bugs.openjdk.org/browse/JDK-8328995 and a PR (https://github.com/openjdk/jdk/pull/18479) was proposed to enhance this C code in the launcher to support ZIP64 jar files. Even before the proposed changes in that PR, there already were a few concerns about long term maintainability of the jar parsing code in the launcher given that it duplicates what we already do in the Java side implementation, so adding new C code here isn't prefer
 able.
> 
> The change in this current PR removes the part where the launcher's C code was parsing the jar's manifest file to identify "Main-Class" and "SplashScreen-Image" attributes from the manifest of the jar that was being launched. This was being done in the C code to support a long outdated "JRE selection" feature (mJRE https://bugs.openjdk.org/browse/JDK-8058407) which required it to parse the jar's manifest. After that feature was removed, the sole reason the jar's manifest was continued to be parsed in this launcher code was for convenience.
> 
> With the changes proposed in this PR, the launcher will use the jar parsing C code only if the jar has a "SplashScreen-Image" in its manifest. The number of such jars, based on an experiment against a large corpus of jar files, is very minimal (we found just 26 jars in around 900K odd jar files with a "SplashScreen-Image"). The updated code in this PR also delegates the identification of the "SplashScreen-Image" attribute to the java code (in `LauncherHelper`) thus removing the need to parse the manifest in C code. The only time the C code will now do the jar parsing is to display a splash screen image (if any is present) from the jar file. I think even that can be avoided, but I haven't experimented with it and I would like to pursue that separately in future, instead of this PR.
> 
> Finally, a few of the utility functions that were introduced in the launcher C code in a recent JEP to support "Implicitly Declared Classes and Instance Main...

Just a few drive-by comments. This is all rather complex from a reviewers perspective - it is very hard to discern what code change relates to what you described in the PR. Can it be broken up into smaller chunks perhaps?

Aside: we do an awful lot of Java code execution in the launcher these days before we even get to load the real main class. I have to wonder how all this affects startup?

src/java.base/macosx/native/libjli/java_md_macosx.m line 88:

> 86:  * the command line or from the manifest of an executable jar file.
> 87:  * The vm selection options are not passed to the running
> 88:  * virtual machine; they must be screened out by the launcher.

The flowchart below seems to be out-of-date as I'm pretty sure no data-model selection is performed any more (no -d64).

src/java.base/share/classes/jdk/internal/misc/MethodFinder.java line 105:

> 103:                                 || !Modifier.isPublic(mods)
> 104:                                 || mainMethod.getParameterCount() != 1)
> 105:                 )) {

This seems a distinct fix rather than a "cleanup". ??

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 607:

> 605:         if (mainAttrs == null) {
> 606:             abort(null, "java.launcher.jar.error3", jarname);
> 607:         }

Why do we no longer need a null check?

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 1046:

> 1044:     // carries details about the application's main method and splash screen image path,
> 1045:     // that will be used by the native code in the launcher.
> 1046:     public record MainEntry (Class<?> mainClass, Method mainMethod,

Nit: no space before `(`.

src/java.base/share/native/libjli/java.c line 62:

> 60:  * A NOTE TO DEVELOPERS: For performance reasons it is important that
> 61:  * the program image remain relatively small until after
> 62:  * CreateExecutionEnvironment has finished its possibly recursive

Is recursion still a possibility?

src/java.base/share/native/libjli/java.c line 1138:

> 1136:         has_arg = IsOptionWithArgument(argc, argv);
> 1137:         if (JLI_StrCCmp(arg, "-version:") == 0) {
> 1138:             JLI_ReportErrorMessage(SPC_ERROR1);

Are these error message definitions, `SPC_ERROR1` etc, unused now?

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

PR Review: https://git.openjdk.org/jdk/pull/20929#pullrequestreview-2299311631
PR Review Comment: https://git.openjdk.org/jdk/pull/20929#discussion_r1756196485
PR Review Comment: https://git.openjdk.org/jdk/pull/20929#discussion_r1756200162
PR Review Comment: https://git.openjdk.org/jdk/pull/20929#discussion_r1756205248
PR Review Comment: https://git.openjdk.org/jdk/pull/20929#discussion_r1756208934
PR Review Comment: https://git.openjdk.org/jdk/pull/20929#discussion_r1756209784
PR Review Comment: https://git.openjdk.org/jdk/pull/20929#discussion_r1756214113