On Thu, 31 Oct 2024 10:52:22 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:
>> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmod java.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmod java.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmod jdk.security.jgss.jmod >> java.datatransfer.jmod java.rmi.jmod java.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmod jdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 172 commits: > > - Merge branch 'master' into jdk-8311302-jmodless-link > - Some test fixes > - Remove period in jlink.properties > - Revert changes to ResourcePoolEntry > - Fix comment in RuntimeImageLinkException > - Remove ImageReader (like JmodsReader) > - More comment fixes (JlinkTask) > - Move some comments around > - More comment fix-ups (JRTArchive) > - Fix description of configure option > - ... and 162 more: https://git.openjdk.org/jdk/compare/c40bb762...b702ba8c Looking quite good. Most comments are minor improvement. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 500: > 498: private static Map<String, List<String>> > recordAndFilterEntries(ResourcePool resultResources) { > 499: final Map<String, List<String>> nonClassResEntries = new > HashMap<>(); > 500: final Platform platform = getTargetPlatform(resultResources); Suggestion: Map<String, List<String>> nonClassResEntries = new HashMap<>(); Platform platform = getTargetPlatform(resultResources); src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 195: > 193: if (secondSlash == -1) { > 194: throw new > AssertionError(); > 195: } Suggestion: assert secondSlash != -1; src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 229: > 227: throw new RuntimeImageLinkException(msg); > 228: } else { > 229: // System.err vs taskHelper.warning? It's my leftover comment. I think this error or warning message should be localized. But this would require to access `JlinkTask::taskHelper`. This needs to consider what is the better way (possibly some other existing cases too) and okay to leave it as is. Suggest to move this comment in line 224 and something like: `// need to consider if this error or message should be localized` src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 336: > 334: } > 335: if (symlinkNum < 0 || symlinkNum > 1) { > 336: throw new IllegalStateException( Suggestion: throw new AssertionError( src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 528: > 526: } catch (IOException e) { > 527: throw new AssertionError("Failed to process resources from > the run-time image" + > 528: " for module " + modName); Suggestion: throw new UncheckIOException("Failed to process resources from the run-time image" + " for module " + modName, e); src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 438: > 436: * > 437: * @param finder A module finder based on packaged modules. > 438: * @param runtimeImageFinder A system modules finder. Suggestion: * @param runtimeImageFinder A system modules finder. * @param finder A module finder based on packaged modules. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/LinkableRuntimeImage.java line 55: > 53: */ > 54: public static boolean isLinkableRuntime() { > 55: try (InputStream in = getDiffInputStream()) { Suggestion: try (InputStream in = getDiffInputStream("java.base")) { Or rename `getDiffInputStream()` to `getJavaBaseDiffInputStream()` but this is simple enough and it's clear above. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/LinkableRuntimeImage.java line 77: > 75: } > 76: return null; > 77: } the callers of this method catches `IOException` and the handler is different. It seems to me that this method should just throw IOException. Returns null only if no diff file for that module is present. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/LinkableRuntimeImage.java line 80: > 78: } > 79: > 80: public static Archive newArchive(String module, Path path, boolean > ignoreModifiedRuntime) { Probably good to add `assert isLinkableRuntime();` to flag that this method is expected to be called only if it has the run-time image linking enabled. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 584: > 582: } > 583: > 584: public void showHelp(String progName, boolean runtimeCap) { Suggestion: public void showHelp(String progName, boolean linkableRuntimeEnabled) { src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 596: > 594: // If the JDK build has the run-time image capability show it > 595: // in the help output > 596: > log.println(bundleHelper.getMessage("main.capability.runtime", Suggestion: log.println(bundleHelper.getMessage("main.runtime.image.linking.capability", src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/JimageDiffGenerator.java line 101: > 99: } > 100: } > 101: // What's now left in optResSet are the resources only present > in the `optResSet` has been renamed. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java line 73: > 71: case 3: return MODIFIED; > 72: } > 73: throw new IllegalStateException("Must not reach here!"); Suggestion: throw new AssertionError("Must not reach here!"); src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java line 133: > 131: if (kind == null || name == null) { > 132: // should this be RuntimeImageLinkException? output > needs cleanup? > 133: throw new IllegalStateException("kind and name must be > set"); It's my leftover comment. Will this happen? Seems not - then should be AssertionError. Same question in line 144-145 src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 114: > 112: \n\ > 113: > 114: main.capability.runtime=Capabilities: {0}run-time-image Suggestion: main.runtime.image.linking.capability=Capabilities: {0}run-time-image src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 123: > 121: \ and cannot be used to create another image with the jdk.jlink module > 122: err.runtime.link.packaged.mods=--keep-packaged-modules is not allowed. > This run-time image capable JDK\ > 123: \ does not include packaged modules I think this error indicates that this JDK does not have any packaged modules, i.e. no default module path `jmods` directory. It may be clearer to say: Suggestion: err.runtime.link.packaged.mods=This JDK has no packaged modules. --keep-packaged-modules is not supported. ------------- PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2406539503 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824885983 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824901335 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824915114 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824905939 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824951594 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824998305 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824972227 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824984959 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824994455 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824959347 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824961610 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824996374 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1824999099 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1825000734 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1825003196 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1825011873
