On Tue, 10 Sep 2024 06:15:57 GMT, Jaikiran Pai <j...@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
