On Wed, 14 Feb 2024 11:03:07 GMT, Christian Stein <cst...@openjdk.org> 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