On Wed, 30 Oct 2024 21:12:42 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 incrementally with 20 additional > commits since the last revision: > > - 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 > - Some more wording updates > - ... and 10 more: https://git.openjdk.org/jdk/compare/83a86d06...e6b3aeb1 src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 151: > 149: public static void recreateJimage(Path jimageFile, > 150: Set<Archive> archives, > 151: ImagePluginStack pluginSupport, boolean generateRuntimeImage) Suggestion: ImagePluginStack pluginSupport, boolean generateRuntimeImage) src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 195: > 193: plugins.storeFiles(allContent.resourcePool(), result, > writer); > 194: } catch (RuntimeImageLinkException e) { > 195: throwRuntimeLinkFailure(e); Curious why this is a special case. Would it be useful for debugging if this throws `new IOException(e)` the same way as other exceptions (line 196-200) so RILE is a cause? src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 255: > 253: } catch (RuntimeImageLinkException re) { > 254: // Might be thrown when linking from the current run-time > image. > 255: // Populate the actual reason. `// Populate the actual reason.` - this comment no longer applies? src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 338: > 336: @SuppressWarnings("try") > 337: private static ResourcePool addResourceDiffFiles(ResourcePool > jmodContent, > 338: ResourcePool > resultContent, BasicImageWriter writer) { Suggestion: ResourcePool resultContent, BasicImageWriter writer) { src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 399: > 397: String mResource = String.format(DIFF_PATH, module); > 398: List<ResourceDiff> diff = perModDiffs.get(module); > 399: if (diff == null) { Can `diff` ever be null? ` preparePerModuleDiffs` ensures that the return map has one entry for each module and empty list if no diff. For the case with no diff, do you expect EMPTY_RESOURCE_BYTES or a `ResourceDiff` content with header & zero entry? Perhaps line 377-386 in `preparePerModuleDiffs` can simply be removed and let `addDiffResourcesFiles` to handle no resource diff case as it does now. Map<String, List<ResourceDiff>> allModsToDiff = new HashMap<>(); modules.stream().forEach(m -> { List<ResourceDiff> d = modToDiff.get(m); if (d == null) { // Not all modules will have a diff allModsToDiff.put(m, Collections.emptyList()); } else { allModsToDiff.put(m, d); } }); return allModsToDiff; src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 435: > 433: > BasicImageWriter writer) { > 434: // Only add resources if we have the jdk.jlink module part of the > 435: // target modules view Suggestion: // Only add resources if jdk.jlink module is present in the target image src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 543: > 541: > 542: private static ResourcePoolManager createBasicResourcePoolManager( > 543: ByteOrder byteOrder, BasicImageWriter writer) { Suggestion: private static ResourcePoolManager createBasicResourcePoolManager(ByteOrder byteOrder, BasicImageWriter writer) { src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 567: > 565: */ > 566: private static ResourcePoolManager createPoolManager( > 567: ResourcePool resultResources, BasicImageWriter writer) { Suggestion: private static ResourcePoolManager createPoolManager(ResourcePool resultResources, BasicImageWriter writer) { ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823546245 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823539718 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823544168 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823545856 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823585738 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823574545 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823578438 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823579074
