On Tue, 4 Nov 2025 17:08:45 GMT, David Beaumont <[email protected]> wrote:
> Adds support for writing preview related flags into jimage files.
>
> Preview mode is complex. It's not nearly as simple as "does something in
> /modules/xxx/... have an entry in /modules/xxx/META-INF/preview/...".
>
> Specific issues include:
>
> Supporting preview-only resources without forcing a double lookup on
> everything.
> Changing the set of entries in /packages/xxx directories to account for
> preview only packages in some modules.
> Minimising the work done during image reader initialization to only need to
> process the small number of preview resources (rather than scanning the whole
> file to look for them).
> The new flags added by this code address these issues, but calculating them
> correctly with only minor adjustments to the existing code was not feasible,
> it just became a lot larger and very complex.
>
> To address this, a new type (ModuleReference) is introduced to track and then
> merge information about packages seen in each module. This allows a much
> simpler inner loop for processing resource paths when building the node tree,
> combined with a subsequent merging stage to produce the final package
> information for each module.
>
> Not that since ModuleReference is needed during jimage reading, that class is
> already present in the previous PR on which this is based, but it starts to
> be used to calculate the module flags in this PR.
>
> This PR can also adds the ImageReader unit tests for preview mode, which rely
> on being able to generate jimage files with preview mode flags in.
src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 89:
> 87: }
> 88: }
> 89: ;
Extra "'"
src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 95:
> 93: // will be always 'true' and this code, and all related dead-code can
> be removed.
> 94: private static final boolean DISABLE_PREVIEW_PATCHING_DEFAULT = false;
> 95: private static final boolean ENABLE_PREVIEW_MODE =
> Boolean.parseBoolean(
the naming is confusing: stick to the same sense as the system property; (Or
change the system property)
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java
line 204:
> 202: try {
> 203: processPath(fullPath, modulesRoot, packageToModules);
> 204: } catch (InvalidTreeException err) {
Exception varialble "err" is ambiguous with System.err used below.
"ex" is more conventional for exceptions.
test/jdk/tools/jlink/whitebox/ImageResourcesTreeTestDriver.java line 24:
> 22: */
> 23:
> 24: /*
Why is this separate from the test itself?
test/micro/org/openjdk/bench/jdk/internal/jrtfs/ImageReaderBenchmark.java line
318:
> 316: }
> 317:
> 318: /// Note: This list is inherently a little fragile and may end up
> being more
Yes, this kind of test/benchmark will bit rot very fast.
The existing startup measurements are more stable and reflect actual startup
times, not a simulation thereof.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1718#discussion_r2492033684
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1718#discussion_r2492038687
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1718#discussion_r2492084995
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1718#discussion_r2492060924
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1718#discussion_r2492057544
