[lworld] Switch JLink to not use ImageReader API #1721

david-beaumont · 2025-11-04T23:34:54Z

Creates a new, narrowed API explicitly for use by jlink, which view the resource entries in a jimage file without the re-mapping of names and invention of synthetic entries inherent in ImageReader.

Another good reason to express this new API as something other than ImageReader is that, to fix issues such as JDK-8357249, we don't want to have the (System)ImageReader class used directly in jlink code. It's just the wrong abstraction and will make it harder to refactor jlink to use a non-singleton API with a controlled lifetime later.

I've not added unit tests for the new API (yet), but the fact the PackagedModulesVsRuntimeImageLinkTest passes with preview content in the jimage file means that it's working as expected.

Progress

Change must not contain extraneous whitespace

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1721/head:pull/1721
$ git checkout pull/1721

Update a local copy of the PR:
$ git checkout pull/1721
$ git pull https://git.openjdk.org/valhalla.git pull/1721/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1721

View PR using the GUI difftool:
$ git pr show -t 1721

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1721.diff

Using Webrev

Link to Webrev Comment

* fixing tests after refactoring * Fixing up after dependent PR changes * feedback and remove unused code * new tests for ImageLocation * Restoring lost changes and updating some comments. * add system property guard to preview mode * Remove TODOs now jimage version is bumped * jimage writer changes to support preview mode.

bridgekeeper · 2025-11-04T23:36:08Z

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into pr/1718 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-11-04T23:37:45Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2025-11-04T23:41:48Z

Webrevs

david-beaumont · 2025-11-04T23:46:18Z

src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java

+     * obtained is closed.
+     */
+    // Package visible for use by ImageReader.
+    ResourceEntries getResourceEntries() {


This is the implementation of the new, narrow, API. That's it.

src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java

david-beaumont · 2025-11-04T23:51:44Z

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java

+     * <p>This API remains valid until the image reader from which it was
+     * obtained is closed.
+     */
+    public ResourceEntries getResourceEntries() {


This is just exposing the [Shared]ImageReader API out via [System]ImageReader.

I'll probably move this to be package protected here and only publicly available via a static method in SystemImage, which really tightens its use to the one use case we have for it right now.

david-beaumont · 2025-11-04T23:53:27Z

src/java.base/share/classes/jdk/internal/jimage/ResourceEntries.java

+ * <p>It disallows access to resource directories (i.e. {@code "/modules/..."}
+ * or packages (i.e. {@code "/packages/..."}.
+ */
+public interface ResourceEntries {


This is an API so that we can avoid using ImageReader directly in the jlink code.
ImageReader is not conceptually the right API for this, and in future we might want to decouple things even more so it's clear that ImageReader is for inspection of resource in a runtime, and something else is there to read the jimage contents directly.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Archive.java

david-beaumont · 2025-11-05T00:03:52Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java

+
+            return new Entry(
+                    archive,
+                    String.format("/%s/%s", archive.moduleName(), resPath),


The problem with the Entry parent class is that it demands a "path", even for cases where none make sense (or at least where the path and name are really the same). This munging of the path to have the module name at the front is what the old code did too, but it really serves no purpose since the path isn't used for this entry.

david-beaumont · 2025-11-05T00:04:40Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java

                @Override
                public long size() {
                    try {
-                        if (resType != EntryType.CLASS_OR_RESOURCE) {


The checks are asserts are needlessly duplicated in both size() and stream() methods.

david-beaumont · 2025-11-05T00:05:44Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java

-
-        //Ask the stack to proceed;
-        stack.operate(imageProvider);
+        try (ImageHelper imageProvider =


ImageHelper holds Archive instances, which should be closed, so putting this in a try-with-resources seems better than what was there.

david-beaumont · 2025-11-05T00:06:31Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java

+        @Override
+        public void close() throws IOException {
+            for (Archive archive : archives) {
+                archive.close();


I could put each close in a try/catch if we were worried about close() actually failing here.

david-beaumont · 2025-11-05T00:08:05Z

test/jdk/tools/jlink/runtimeImage/PackagedModulesVsRuntimeImageLinkTest.java

        }
    }

+    // Helper to assert the content of two jimage files are the same and provide


This is an optional (but nice) improvement to how the test reported mismatched lists of entries.
Without it, the test just fails with no useful output.
If anyone knows a neater way (e.g. some set difference methods) then I'll happily neaten this up.

I tidied it up (should have done this from the start).

src/java.base/share/classes/jdk/internal/jimage/SystemImageReader.java

david-beaumont · 2025-11-05T02:09:12Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Archive.java

+         * Constructs an entry of the given archive.
+         *
+         * @param archive the archive in which this entry exists.
+         * @param path the complete path of the entry, including the module.


Note that now I removed the path() method I added, the path passed here serves absolutely no purpose other than to appear in the toString() output. I'd vote for removing it completely and simplifying callers.

david-beaumont · 2025-11-05T13:10:37Z

src/java.base/share/classes/jdk/internal/jimage/ResourceEntries.java

@@ -0,0 +1,71 @@
+/*


This is a copy of the notice in BasicImageReader, so I assume the clause about the GPL is correct.
Please let me know if not.

…ashed

* copyright update * feedback changes

RogerRiggs · 2025-11-05T14:24:01Z

src/java.base/share/classes/jdk/internal/jimage/ResourceEntries.java

+ * jimage files directly. For inspection of runtime resources, it is vital that
+ * {@code previewMode} is correctly observed, making this API unsuitable.


This would be more clearly stated as use API xxx to access the classes and resources of a runtime.

RogerRiggs · 2025-11-05T14:30:28Z

src/java.base/share/classes/jdk/internal/jimage/ResourceEntries.java

+     * random order. Entry names will always be prefixed by the given module
+     * name (e.g. {@code "/<module-name>/..."}).
+     */
+    Stream<String> entryNamesIn(String module);


Are these names useful with any ImageReader api other than sizeOf or open?

RogerRiggs · 2025-11-05T14:33:15Z

src/java.base/share/classes/jdk/internal/jimage/ResourceEntries.java

+     * random order. Entry names will always be prefixed by the given module
+     * name (e.g. {@code "/<module-name>/..."}).
+     */
+    Stream<String> entryNamesIn(String module);


Those extra words at the end of the method name are not consistent with naming conventions in openjdk.
Drop the "In".

RogerRiggs · 2025-11-05T14:42:23Z

src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java

+                // Other types of invalid name just result in no entry being found.
+                if (name.startsWith("/modules/") || name.startsWith("/packages/")) {
+                    throw new IllegalArgumentException("Invalid entry name: " + name);
+                }


This is already covered by the == null case below. The distinguished exception isn't necessary.

RogerRiggs · 2025-11-07T15:48:35Z

test/jdk/tools/jlink/runtimeImage/PackagedModulesVsRuntimeImageLinkTest.java

 * @build tests.* jdk.test.lib.process.OutputAnalyzer
 *        jdk.test.lib.process.ProcessTools
- * @run main/othervm/timeout=1200 -Xmx1g PackagedModulesVsRuntimeImageLinkTest
+ * @run main/othervm/timeout=1200 -ea -esa -DDISABLE_PREVIEW_PATCHING=false -Xmx1g PackagedModulesVsRuntimeImageLinkTest


Should this test (temporarily) be run both with and without the new packaging?

openjdk-notifier · 2025-11-11T13:21:16Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout jdk_8371292_jlink
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge lworld"
git push

openjdk · 2025-11-11T13:23:10Z

@david-beaumont this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout jdk_8371292_jlink
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

openjdk · 2025-11-11T13:23:12Z

⚠️ @david-beaumont This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

david-beaumont added 2 commits November 4, 2025 18:03

Make jlink use a new narrow API for raw jimage access

3fcfea6

openjdk bot added the rfr Pull request is ready for review label Nov 4, 2025

Remove redundant extra method (part of original prototype)

a433f09

david-beaumont commented Nov 5, 2025

View reviewed changes

Tighted access to new API and undo new path() method.

67984e2

david-beaumont commented Nov 5, 2025

View reviewed changes

src/java.base/share/classes/jdk/internal/jimage/SystemImageReader.java Show resolved Hide resolved

david-beaumont commented Nov 5, 2025

View reviewed changes

Added copyright notice and tidied a couple of comments.

a2f0c0c

david-beaumont commented Nov 5, 2025

View reviewed changes

david-beaumont added 8 commits November 5, 2025 15:11

Tidied up diff code in test.

fdcb73f

Minor feedback changes

e52692f

Feedback and test fix

533e5af

fixing jlink issue

7263e2e

Rename ModuleReference methods back.

8e702fa

Merge branch 'jdk_8368467_reader/squashed' into jdk_8371292_jlink/squ…

1f85ff7

…ashed

Feedback changes.

726ace9

* copyright update * feedback changes

comment adjustment

de2c1be

RogerRiggs reviewed Nov 7, 2025

View reviewed changes

openjdk-notifier bot changed the base branch from pr/1718 to lworld November 11, 2025 13:21

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 11, 2025

		* jimage files directly. For inspection of runtime resources, it is vital that
		* {@code previewMode} is correctly observed, making this API unsuitable.

[lworld] Switch JLink to not use ImageReader API #1721

Are you sure you want to change the base?

[lworld] Switch JLink to not use ImageReader API #1721

Uh oh!

Conversation

david-beaumont commented Nov 4, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Reviewing

Uh oh!

bridgekeeper bot commented Nov 4, 2025

Uh oh!

openjdk bot commented Nov 4, 2025

Uh oh!

mlbridge bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk-notifier bot commented Nov 11, 2025

Uh oh!

openjdk bot commented Nov 11, 2025

Uh oh!

openjdk bot commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

david-beaumont commented Nov 4, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 4, 2025 •

edited

Loading