RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v42]
Mandy Chung
mchung at openjdk.org
Thu Oct 31 18:43:57 UTC 2024
On Thu, 31 Oct 2024 10:52:22 GMT, Severin Gehwolf <sgehwolf at 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
More information about the build-dev
mailing list