On Thu, 12 Mar 2026 05:12:11 GMT, Jaikiran Pai <[email protected]> wrote:
>> Implementation of preview-mode support for jimage modules file, migrated
>> from Valhalla related work (see JDK-8352750).
>>
>> This PR (the first of several) migrates work from Valhalla (lworld) to the
>> JDK mainline repository in relation to "preview mode" support. It affects
>> the creation and reading of the jimage file, both in Java
>> (BasicImageReader/ImageReader) and C++ (imageFile.xpp/jimage.xpp).
>>
>> Preview mode is a mechanism by which alternate version of JDK class files
>> and resources can be made available for class loading and reflection when
>> the '--enable-preview' flag is passed to the runtime.
>>
>> Alternate classes/resource appear in each module under the:
>>
>> /<module>/META-INF/preview/<path-to>/<resource-or-class>
>>
>> and replace the original:
>>
>> /<module>/<path-to>/<resource-or-class>
>>
>> files when preview mode is enabled.
>>
>> While initially useful for Valhalla work, this mechanism will be used for
>> other cases where preview features (in the JEP 12 sense) require alternate
>> classes/resources to be provided. None of the changes in this (or the
>> follow-up PRs) are Valhalla specific.
>>
>> In this PR:
>> * the writing of jimage files is modified to recognize and handle preview
>> mode paths
>> * flags in the jimage file are added or modified to support preview mode
>> efficiently
>> * (C++) the class loader is modified to permit reading preview versions of
>> classes
>> * (Java) the image reader and associated JRT file-system classes are
>> modified to permit reading preview files
>> * unit tests are added to ensure preview mode works as expected when enabled
>> * (temporary) any code calling into the affected API (other than tests)
>> specifies that preview mode is disabled
>>
>> Future PRs will add the plumbing to enable preview mode correctly, but with
>> the PR there should be no observable change in behaviour (especially since
>> no preview classes or resources are being supplied at this point).
>
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 237:
>
>> 235: public ReaderKey(Path imagePath, boolean previewMode) {
>> 236: this.imagePath = imagePath;
>> 237: this.previewMode = previewMode;
>
> This doesn't appear to be a mode, should it be named `previewEnabled` instead?
Ahh yes, that's a bit clearer.
> src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 53:
>
>> 51: * {@code --enable-preview} flag.
>> 52: */
>> 53: FOR_RUNTIME;
>
> Nit - would `OF_RUNTIME` be a better name?
They seem to be synonymous to me.
> src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 59:
>
>> 57: */
>> 58: public boolean isPreviewModeEnabled() {
>> 59: // A switch, instead of an abstract method, saves 3 subclasses.
>
> I think the use of `switch` is good and natural. However, I think this
> comment is a bit distracting (for example, when I read it I didn't understand
> what abstract method or subclasses it was talking about and then had to pay
> much more attention to the switch block to see if anything
> interesting/unexpected was going on there).
This comment was added in response to a comment when the code was first
written. Since Abstract classes are a common pattern for adding this sort of
functionality, I wanted to point out the "cons" of switching to it to any
future maintainer who might be tempted.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923794538
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923777682
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923788850
