On Wed, 14 Feb 2024 11:03:07 GMT, Christian Stein <[email protected]> wrote:
> Please review this PR that makes the launcher helper keep a reference to the
> executable JAR file active after extracting the name of the main class and
> returning it as Class instance. Now, when loading classes from the JAR file,
> it hasn't to be re-opened.
So if we run today with `java -jar app.jar` then the JAR file will be opened,
and its manifest parsed, by LauncherHelper.loadMainClass, then again by
URLClassPath$JarLoader when it is lazily opened on the class path. That seems
reasonable. It would of course be useful to see any performance/startup data
but it might be hard to gather.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 593:
> 591: }
> 592:
> 593: private static String getMainClassFromJar(String jarname, JarFile
> jarFile) {
Can you check if the "jarname" parameter can be dropped, the error handling in
this method should be able to use jarFile.getName().
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 596:
> 594: String mainValue;
> 595: try {
> 596: Manifest manifest = jarFile.getManifest();
I think the try-catch around the block can be dropped and you can put a more
targeted try-catch around getManifest, at least I think that is the only case
remaining in getMainClassFromJar that needs error handling now.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 823:
> 821: // get the class name
> 822: String cn;
> 823: // store the jar file
"store the jar file" is a confusing comment to add. I think what you want to
say is that the JarFile will put the underlying file in the JarFile/ZipFile
cache and this will avoid needing to re-parse the manifest when the JAR file is
opened on the class path, triggered by Class.forName.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 876:
> 874: jarFile.close();
> 875: } catch (IOException ioe) {
> 876: abort(ioe, "java.launcher.jar.error1", what);
java.launcher.jar.error1 is "Error: An unexpected error occurred while trying
to open file". I can't think of any cases where it might fail but the error
message would be confusing if it did.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17843#pullrequestreview-1880069738
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489338851
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489340464
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489339435
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489342147
