On Fri, 25 Oct 2024 16:29:52 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 five
> additional commits since the last revision:
>
> - Better handle patched modules
>
> Also add a test which ensures that module patching (if present), will
> get an appropriate error message.
> - Add /othervm to some langtools tier1 tests
>
> Those tests are using module patches from JTREG. Since the run-time
> image based link uses ModuleFinder.ofSystem(), which will see the extra
> classes comming from the module patch. Then the size look-up using the
> JRT FS provider fails, since that only looks in the module image
> (lib/modules) and NOT also in the patch. Thus, we get a
> NoSuchFileException and the link fails.
>
> Run the tests with othervm so that the JTREG patch'ed module isn't
> visible to the test.
> - Fix tests for builds with --enable-linable-runtime
>
> Those builds don't include the packaged modules, `jmods` directory.
> However, some tests assume that they're there. Add appropriate requires
> tag.
> - Fix provider verification when some JMODs are present
>
> In some configurations, e.g. when java.base is missing from the packaged
> modules, but another JDK module is present as JMOD that is linked into
> an image, then provider verification can fail. Thus, the run-time image
> link fails. Verify that this doesn't happen.
>
> The fix is to return Platform.runtime() for run-time image based links
> as well. Otherwise this code would return the wrong result.
> - Show run-time image based capability in help
>
> Also add a test for it when it's turned on and off.
I did a pass of the current implementation (not the tests). Looking quite
good.
I see you add the last line in the help message.
Capabilities: +run-time-image
This needs some discussion/consideration how that information be conveyed.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line
111:
> 109: * @throws IOException
> 110: */
> 111: public static ExecutableImage create(Set<Archive> archives,
I think these `create(Set<Archive>, ImagePluginStack)` and
`create(Set<Archive>, ByteOrder)` methods are unused. Can just remove them.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 104:
> 102: this.errorOnModifiedFile = errorOnModifiedFile;
> 103: this.otherRes = readModuleResourceFile(module);
> 104: this.resDiff =
> prepareDiffMap(Objects.requireNonNull(perModDiff));
Suggestion:
this.resDiff = Objects.requireNonNull(perModDiff).stream()
.collect(Collectors.toMap(ResourceDiff::getName,
Function.identity()));
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 183:
> 181: null /*
> hashOrTarget */,
> 182: false /* symlink
> */,
> 183:
> resDiff.get(lookupKey));
Nit: identation within `{...}` better to use consistent spacing (4-space or
8-space is fine).
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 197:
> 195: String
> pathWithoutModule = s.getName()
> 196:
> .substring(secondSlash + 1,
> 197:
> s.getName().length());
Suggestion:
String pathWithoutModule =
s.getName().substring(secondSlash + 1);
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 205:
> 203: s);
> 204: })
> 205: .collect(Collectors.toList()));
Suggestion:
.toList());
Can replace `.collect(Collectors.toList())` with `Stream::toList` in other
occurrances as well.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 326:
> 324: try {
> 325: Integer typeInt = Integer.valueOf(tokens[0]);
> 326: type = Type.fromOrdinal(typeInt);
Since encoding and decoding are private to this class, this class can keep a
map from an ordinal to `Type` and no need to add the static `Type::fromOrdinal`
method.
private static final Map<Integer, Type> typeMap =
Arrays.stream(Type.values())
.collect(Collectors.toMap(Type::ordinal, Function.identity()));
Suggestion:
type = typeMap.get(typeInt);
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 526:
> 524: }
> 525: } catch (IOException e) {
> 526: throw new InternalError("Failed to process run-time image
> resources " +
For unexpected exceptions, some code throws InternalError and some throws
AssertionError - better to be consistent and I think implementation typically
throws InternalError. For IOException, I think `UncheckedIOException` is
appropriate.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 246:
> 244: public static final String RESPATH_PATTERN =
> "jdk/tools/jlink/internal/runtimelink/fs_%s_files";
> 245: // The diff files per module for linkable JDK runtimes
> 246: public static final String DIFF_PATTERN =
> "jdk/tools/jlink/internal/runtimelink/diff_%s";
I think it'd be useful to have a `LinkableRuntimeImage` class that declares
these constants and some other utility methods such as detecting if this is a
linkable runtime image (e.g. `hasRuntimeImageLinkCap()`)
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 414:
> 412: }
> 413: // Setup and init actions for JDK linkable runtimes
> 414: LinkableRuntimesResult result = linkableJDKRuntimesInit(finder,
> roots);
I think it's clearer to the readers if inlining the code of
`linkablejDKRuntimesInit` here. Also the `--keep-package-modules` check can be
moved after `isLinkFromRuntime` is determined so checking `isLinkFromRuntime`
is more explicit.
Suggestion:
boolean isLinkFromRuntime = options.modulePath.isEmpty();
// In case of custom modules outside the JDK we may
// have a non-empty module path, which must not include
// java.base. If it did, we link using packaged modules from that
// module path. If the module path does not include java.base, we must
// have a linkable JDK runtime. In that case we take the JDK modules
// from the run-time image.
if (finder.find("java.base").isEmpty()) {
isLinkFromRuntime = true;
ModuleFinder runtimeImageFinder = ModuleFinder.ofSystem();
finder = combinedFinders(runtimeImageFinder, finder,
options.limitMods, roots);
}
// --keep-packaged-modules doesn't make sense as we are not linking
// from packaged modules to begin with.
if (isLinkFromRuntime && options.packagedModulesPath != null) {
throw taskHelper.newBadArgs("err.runtime.link.packaged.mods");
}
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 480:
> 478: */
> 479: private ModuleFinder combinedFinders(ModuleFinder finder,
> 480: ModuleFinder runtimeImageFinder, Set<String> limitMods,
suggest to make `runtimeImageFinder` as the first parameter and `finder` second
to match the order of the lookup.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 486:
> 484: @Override
> 485: public Optional<ModuleReference> find(String name) {
> 486: Optional<ModuleReference> basic =
> runtimeImageFinder.find(name);
Suggestion:
Optional<ModuleReference> mref = runtimeImageFinder.find(name);
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 559:
> 557: Runtime.Version version = Runtime.version();
> 558: Path[] entries = paths.toArray(new Path[0]);
> 559: ModuleFinder finder = paths.isEmpty() ? ModuleFinder.ofSystem()
The javadoc needs update to something like:
* Returns a module finder of the given module path or system modules
* if the module path is empty that limits the observable modules to ...
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 741:
> 739: // we don't have the per module resource diffs in the modules
> image
> 740: try (InputStream inStream = getDiffInputStream()) {
> 741: if (inStream == null) {
Suggest to define a method `isLinkableRuntime` in the new
`LinkableRuntimeImage` class to test if it's a linkable runtime image.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 760:
> 758: }
> 759:
> 760: private static boolean hasRuntimeImageLinkCap() {
Move to the new `LinkableRuntimeImage` class
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 825:
> 823: }
> 824: } else if (config.linkFromRuntimeImage()) {
> 825: // This is after all other archive types, since user-provided
This block can be a utility method in the new `LinkableRuntimeImage` class as
the logic can be encapsulated.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ImageReader.java
line 39:
> 37: }
> 38:
> 39: public static boolean isNotTreeInfoResource(String path) {
Do we need this? Can use `ImageResourcesTree::isTreeInfoResource` instead
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ImageReader.java
line 48:
> 46: .filter(ImageReader::isNotTreeInfoResource)
> 47: .sorted()
> 48: .collect(Collectors.toList());
Suggestion:
return Arrays.stream(getEntryNames())
.filter(Predicate.not(ImageResourcesTree::isTreeInfoResource))
.sorted()
.toList();
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/JmodsReader.java
line 41:
> 39:
> 40: @SuppressWarnings("try")
> 41: public class JmodsReader implements JimageDiffGenerator.ImageResource {
Is JmodsReader leftover from previous implementation? It can be removed?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
line 272:
> 270:
> 271: public static void printDiffs(List<ResourceDiff> diffs) {
> 272: for (ResourceDiff diff:
> diffs.stream().sorted().collect(Collectors.toList())) {
Suggestion:
for (ResourceDiff diff: diffs.stream().sorted().toList()) {
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/RuntimeImageLinkException.java
line 29:
> 27:
> 28: /**
> 29: * Exception thrown for links without packaged modules. I.e. run-image
> link.
I found inconsistent terminologies "run-image" "run-time" "JDK run-time".
Better to use consistent terminologies.
For this one, maybe as simple as this:
Suggestion:
* Exception thrown for linking without packaged modules
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/RuntimeImageLinkException.java
line 38:
> 36: private final IllegalArgumentException iae;
> 37:
> 38: public RuntimeImageLinkException(IllegalArgumentException cause) {
Can you explain why `RuntimeImageLinkException` constructor takes IAE. The
places throwing RILE has to create IAE. I can't see how IAE is necessary as
other places would catch RILE and throw IAE or IOException instead.
Can it simply take an exception message parameter? And possibly change
`JlinkTask::run` in handling RILE like IAE?
} catch (IllegalArgumentException | ResolutionException |
RuntimeImageLinkException e) {
log.println(taskHelper.getMessage("error.prefix") + " " +
e.getMessage());
if (DEBUG) {
e.printStackTrace(log);
}
return EXIT_ERROR;
```
src/jdk.jlink/share/classes/jdk/tools/jlink/plugin/ResourcePoolEntry.java line
73:
> 71: TOP;
> 72:
> 73: public static Type fromOrdinal(int value) {
Can be dropped if `JRTArchive.ResourceFileEntry` keeps a map from ordinal to
Type instances.
src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 119:
> 117: warn.prefix=Warning:
> 118:
> 119: err.runtime.link.not.linkable.runtime=The current run-time image does
> not support run-time linking.
should we use "runtime" instead of "run-time" in the help message and
errors/warnings? "runtime" is already used in the messages in this file.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2396177744
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817204284
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817236999
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817238678
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817241483
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817240162
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817246303
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817251038
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817219627
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817225268
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817226390
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817229066
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817228237
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817230995
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817231628
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817233166
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817253259
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817253657
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817205476
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817261753
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817208860
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817216753
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817265479
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1817269156
