On Tue, 21 Oct 2025 19:43:04 GMT, Roger Riggs <[email protected]> wrote:
>> David Beaumont has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 12 additional >> commits since the last revision: >> >> - Rolled up changes after rebase. >> >> * Removing package root flag based on feedback. >> * Changing existing package flags during writing to match altered flag >> values. >> * Feedback changes, and fixing some comments. >> * Renaming slightly confusing "testEncoder" method. >> * Fixing unit tests to use new constructor. >> * Word smithing flags definitions. >> * Add workaround until new image writing code is in >> * Clarifying flag docs for /packages/xxx case >> * Java ImageReader changes for preview mode >> - Merge branch 'jdk_8366093_cpp/squashed' into jdk_8368333_java/squashed >> - [[RESET BRANCH FOR MERGE]] >> - Removing package root flag based on feedback. >> - Changing existing package flags during writing to match altered flag >> values. >> - Feedback changes, and fixing some comments. >> - Test fixes and feedback changes. >> >> * Renaming slightly confusing "testEncoder" method. >> * Fixing unit tests to use new constructor. >> - Manually deleting ImageReaderFactory (it returned somehow) >> - Word smithing flags definitions. >> - Add workaround until new image writing code is in >> - ... and 2 more: >> https://git.openjdk.org/valhalla/compare/802d18fe...9bbc26c1 > > src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 282: > >> 280: // preview-only nodes. This is used to add preview-only content >> to >> 281: // directories as they are completed. >> 282: private final HashMap<String, Directory> >> previewDirectoriesToMerge; > > Can this be cleared or freed after the ImageReader is open. Its no longer > needed. It's used when directories are completed (as stated in the comment): List<Node> previewOnlyNodes = getPreviewNodesToMerge(dir); ... children.addAll(previewOnlyNodes); > src/java.base/share/classes/jdk/internal/jimage/ImageStrings.java line 38: > >> 36: // String offset constants are useful for efficient classification >> 37: // of location entries without string comparison. These may change >> 38: // between jimage versions (they are checked during initialization). > > The checking seem only to be done when ImageStringsWriter is > loaded/initialized. > There may be a small risk that a mismatch of the image with the > ImageStringsReader could occur. > Can they be checked also by the reader? Hmm, I don't see a nice way to do this at runtime since you can't lookup string offsets by their value (as far as I can see). It's no more fragile than the old code (which had hard-coded constants for "class" and "" (and wasn't self-checking their validity). At least now it's tested at build time. If the version number matches it should be safe. I'll add a comment about how changing existing entries is problematic. > src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 120: > >> 118: * under many modules, it only has content in one. >> 119: */ >> 120: public boolean hasContent() { > > As suggested, `hasResoruces` would be consistent with the lookups for > resources. Will do. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2453165416 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2453158966 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2453160430
