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.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 452:
> 450: String addOptionsGlob = "glob:" +
> AddOptionsPlugin.OPTS_FILE;
> 451: String saveJlinkOptsGlob = "glob:/jdk.jlink/" +
> JlinkTask.OPTIONS_RESOURCE;
> 452: String additionalPatterns = systemModulesPattern + "," +
Suggest to have each plugin to define `Plugin::excludeResourcesPattern` to
return a non-empty string if linking from the run-time image. Construct this
pattern using the API.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 475:
> 473: // SystemModulesMap class isn't guaranteed to be correct
> for the
> 474: // current module set.
> 475: if (systemModulesPlugin == null) {
Suggest to fail if system modules plugin does not exist which should not
happen. The `disableFastPath` system property was added for testing only.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 56:
> 54: * resources. Needed for the the run-time image based jlink.
> 55: */
> 56: public final class AddRunImageResourcesPlugin extends AbstractPlugin {
This resource file is generated if jdk.jlink is linked in the resulting image.
Maybe this plugin can be named `GenerateJlinkResourcesListPlugin` or
`JlinkResourcesListPlugin`???
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 67:
> 65: // RunImageArchive for further processing.
> 66: private static final String RESPATH = RESPATH_PREFIX + "%s_resources";
> 67: private static final String JLINK_MOD_NAME = "jdk.jlink";
There are only 2 occurrences of "jdk.jlink". I guess you plan to replace them
with the constant variable?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 102:
> 100: Platform targetPlatform = getTargetPlatform(in);
> 101: in.transformAndCopy(e -> { ResourcePoolEntry retval =
> recordAndFilterEntry(e, targetPlatform);
> 102: return retval;}, out);
Suggestion:
in.transformAndCopy(e -> recordAndFilterEntry(e, targetPlatform),
out);
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 116:
> 114: if (platform == null) {
> 115: throw new IllegalStateException("java.base not part of the
> image?");
> 116: }
Can simply use `orElseThrow`. It should not reach here and so it's ok to use
InternalError or AssertionError (which is also used in this code).
Suggestion:
String platform = in.moduleView().findModule("java.base")
.map(ResourcePoolModule::targetPlatform)
..orElseThrow(() -> new AssertionError("java.base not found"));
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 121:
> 119:
> 120: private void addModuleResourceEntries(ResourcePoolBuilder out) {
> 121: for (String module: keysInSortedOrder()) {
Suggestion:
nonClassResEntries.keySet().stream().sorted().forEach(module -> {
`keysInSortedOrder` can be removed.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 126:
> 124: if (mResources == null) {
> 125: throw new AssertionError("Module listed, but no
> resources?");
> 126: }
Since it is always non-null, this check can be dropped. NPE will be thrown if
such bug exists.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 159:
> 157: // Filter internal runtime image based link resource file
> which we
> 158: // create later on-the-fly
> 159: return null;
why this one is not excluded in the same way as other plugins?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
line 184:
> 182: } catch (RunImageLinkException e) {
> 183: // RunImageArchive::RunImageFile.content() may throw this
> when
> 184: // getting the content(). Propagate this specific exeption.
Suggestion:
// getting the content(). Propagate this specific exception.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408528283
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408529843
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408525936
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408457185
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408453177
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408463342
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408470820
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408469356
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408472411
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1408475136
