On Fri, 24 Nov 2023 13:25:35 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:
>> Please review this patch which adds a "jmodless" jlink mode to the JDK.
>> 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
>> `JmodLessArchive` 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 java.se)
>> <(../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.internal.vm.compiler.management.jmod
>> jdk.jlink.jmod jdk.naming.dns.j...
>
> Severin Gehwolf has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tighten ModifiedFilesExitTest
>
> Ensure the error message is reasonable and doesn't include
> Exceptions presented to the user.
Thanks. I'll continue the review on the revised version.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 88:
> 86:
> 87: // Run time based link internal resources files
> 88: private static final String OTHER_RESOURCES_FILE =
> "jdk/tools/jlink/internal/runlink_%s_resources";
This can be shared with the plugin writing this file. This file collects the
hash of the non-class resource files in the image. "runlink" is a confusing
prefix. What about:
Suggestion:
// jlink's internal resource file keeps track of the hash of per-module
non-class resources in the run-time image
public static final String MODULE_RESOURCES_LIST =
"jdk/tools/jlink/internal/%s_resources";
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 286:
> 284: }
> 285:
> 286: boolean useModulePath = !options.modulePath.isEmpty();
Is there any reason why the run-time image based linking is only supported if
`--module-path` is not specified? Linking user modules specified on the
module path with the run-time image seems useful. If the module exists in the
given module path and the run-time image, the modules from the module path
should probably take precedence but it needs to work through the details and
potential issues.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 561:
> 559: // Print info message when a run-image link is being
> performed
> 560: if (log != null) {
> 561: log.println("'jmods' folder not present, performing a
> run-time image based link.");
This should be a localized message defined in `jlink.properties`.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 570:
> 568: .sorted(Comparator.comparing(ResolvedModule::name))
> 569: .forEach(rm -> log.format("%s %s%s%n",
> 570: rm.name(),
> rm.reference().location().get(), config.useModulePath() ? "" : " (run-time
> image)"));
"run-time image" should also be a localized message.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 621:
> 619: String resName = String.format(OTHER_RESOURCES_FILE, modName);
> 620: try {
> 621: InputStream inStream = jdkJlink.getResourceAsStream(resName);
should it always create the resouorce file for each module even it's empty that
might make it easier to catch any issue?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/RunImageArchive.java line
47:
> 45: import jdk.tools.jlink.plugin.ResourcePoolEntry.Type;
> 46:
> 47: public class RunImageArchive implements Archive {
What about `JimageArchive` or `JRTArchive`? while jimage does not include the
files not in the jimage, it may be okay since that contains most of the files.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/RunImageArchive.java line
52:
> 50: // File marker in lib/modules file for java.base indicating it got
> created
> 51: // with a run-image-type link.
> 52: private static final String RUNIMAGE_SINGLE_HOP_STAMP =
> ".runtimeimage.stamp";
This marker file is only used by jlink. So it can be in
`jdk.jlink/jdk/tools/jlink/internal/runtimeimage.link.stamp`.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/RunImageArchive.java line
332:
> 330: throw new IllegalArgumentException("Unknown type: " +
> input);
> 331: }
> 332: }
Suggestion:
return switch (input) {
case CLASS_OR_RESOURCE ->
Archive.Entry.EntryType.CLASS_OR_RESOURCE;
case CONFIG -> Archive.Entry.EntryType.CONFIG;
case HEADER_FILE -> Archive.Entry.EntryType.HEADER_FILE;
case LEGAL_NOTICE -> Archive.Entry.EntryType.LEGAL_NOTICE;
case MAN_PAGE -> Archive.Entry.EntryType.MAN_PAGE;
case NATIVE_CMD -> Archive.Entry.EntryType.NATIVE_CMD;
case NATIVE_LIB -> Archive.Entry.EntryType.NATIVE_LIB;
case TOP -> throw new IllegalArgumentException("TOP files
should be handled by ReleaseInfoPlugin!");
default -> throw new IllegalArgumentException("Unknown type: "
+ input);
};
-------------
PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1832864251
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409934416
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409918050
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409926488
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409927957
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409938120
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409946933
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409984852
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1409987063
