On Tue, 14 Oct 2025 18:28:48 GMT, David Beaumont <[email protected]> wrote:
>> Java changes for supporting preview mode when preview mode resources (with >> new location flags) are available. >> >> At the moment, this code will operate on non-preview jimage files (1.0) and >> act as if no preview resources are available by virtue of the default value >> for missing attributes and package flags being zero (which matches jimage >> 1.0). >> >> This should be reviewed on top of >> https://github.com/openjdk/valhalla/pull/1618 > > 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/60b6ed62...9bbc26c1 src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 60: > 58: public static final int MAJOR_VERSION = 1; > 59: public static final int MINOR_VERSION = 1; > 60: private static final int HEADER_SLOTS = 7; Please add a comment before these constants connecting them to imageFile.hpp. src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 48: > 46: public static final int ATTRIBUTE_COMPRESSED = 6; > 47: public static final int ATTRIBUTE_UNCOMPRESSED = 7; > 48: public static final int ATTRIBUTE_PREVIEW_FLAGS = 8; Please add a comment above these constants that the values are defined in ImageFile.hpp. 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. src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 525: > 523: } > 524: ImageLocation loc = findLocation(moduleName, resourcePath); > 525: return loc != null && loc.getType() == RESOURCE; Would there be any benefit to caching the resource here, since it has been found and will likely be opened again. 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? src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 57: > 55: /** If set, the associated module has resources (in normal or preview > mode). */ > 56: // TODO: Make this private again when image writer code is updated. > 57: public static final int FLAGS_HAS_CONTENT = 0x4; Please change the prefix of these to distinguish them from the ImageLocation flags. For example, "PKG_" or "MR_". 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. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2449397419 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2449382931 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2449520220 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2449546445 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2449424904 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2449391627 PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2449493953
