On Tue, 4 Nov 2025 23:48:46 GMT, David Beaumont <[email protected]> wrote:
>> 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](https://bugs.openjdk.org/browse/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. > > David Beaumont has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant extra method (part of original prototype) src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java line 343: > 341: */ > 342: // Package visible for use by ImageReader. > 343: ResourceEntries getResourceEntries() { This is the implementation of the new, narrow, API. That's it. src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java line 353: > 351: .map(offsets::get) > 352: .filter(offset -> offset != 0) > 353: // Reusing a location instance or getting the > module It's important to put the filtering as early as possible since callers are going to enumerate ~30,000 entries to get back maybe 50 entry names in a module. We DO NOT want to sort here, because the caller might (and does) do more filtering before getting the final set. If there's any performance issues (which I doubt given that this is build-time stuff) there are ways to reduce the number of ImageLocation instances being churned. src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 242: > 240: * obtained is closed. > 241: */ > 242: 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. src/java.base/share/classes/jdk/internal/jimage/ResourceEntries.java line 20: > 18: * or packages (i.e. {@code "/packages/..."}. > 19: */ > 20: 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 line 38: > 36: * other, for a module. > 37: */ > 38: public interface Archive extends Closeable { Archives have a close() method and nobody was ever calling it. Seems good to try and address that a bit. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Archive.java line 89: > 87: > 88: /** > 89: * Returns the path of this entry. Don't know why this wasn't a method before, and if there's a conceptual issue with it being as method, I can work around it, but this is cleaner. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 179: > 177: // Add classes/resources from the run-time image, > 178: // patched with the run-time image diff > 179: imageResources.entryNamesIn(module) No more mucking about with String.format() here since the APIs are better aligned and everything is using the full entry name. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 305: > 303: > 304: /** > 305: * line: {@code <int>|<int>|<hashOrTarget>|<path>} Tidying bad HTML JavaDoc in passing since it renders terribly in an IDE. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 418: > 416: } > 417: > 418: record JrtModuleFile( There are really two dinstinct types of files here; those inside a module, and those read from the "root" namespace. And for module files, there are two cases; those defined by a "diff" and those read from the jimage. The old code mushed all of this together so that the size() and stream() methods make runtime checks to decide what to do each time they're called, despite the role being fixed when they are created. It also used a single record with the union of all necessary parameters in it. The new code split each case into its own, very simple, class with any decisions about the role being made before its created. The code has less duplication of checks and is simpler to reason about. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 433: > 431: assert diff.getName().equals(resPath); > 432: > 433: return new Entry(archive, resPath, resName, > EntryType.CLASS_OR_RESOURCE) { The returned Entry instances are now free of if-statements and each does exactly one well defined job that's easy to see at a glance. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 456: > 454: public long size() { > 455: try { > 456: if (resType != EntryType.CLASS_OR_RESOURCE) { The checks are asserts are needlessly duplicated in both size() and stream() methods. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 478: > 476: return new Entry( > 477: archive, > 478: 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. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 352: > 350: > 351: // First create the image provider > 352: 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. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 1058: > 1056: public void close() throws IOException { > 1057: for (Archive archive : archives) { > 1058: archive.close(); I could put each close in a try/catch if we were worried about close() actually failing here. test/jdk/tools/jlink/runtimeImage/PackagedModulesVsRuntimeImageLinkTest.java line 148: > 146: } > 147: > 148: // 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. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492341607 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492347237 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492352870 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492355482 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492356659 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492357728 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492359299 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492360202 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492368103 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492361952 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492371879 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492370888 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492373080 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492374175 PR Review Comment: https://git.openjdk.org/valhalla/pull/1721#discussion_r2492376185
