RFR: 8368333: [lworld] Add preview mode to ImageReader and JRT file-system [v3]

Mon Sep 29 01:54:00 UTC 2025

On Tue, 23 Sep 2025 23:06:28 GMT, David Beaumont <duke at openjdk.org> 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 incrementally with one additional commit since the last revision:
> 
>   Manually deleting ImageReaderFactory (it returned somehow)

src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 42:

> 40:  * <ul>
> 41:  *     <li>src/java.base/share/native/libjimage/imageFile.hpp
> 42:  * </ul>

Are there any java source files that have to be updated on a version change?

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 56:

> 54:     /**
> 55:      * Indicates that a non-preview location is associated with preview
> 56:      * resources.

This comment would read better saying that preview resources exist related to this location.
More like FLAGS_HAS_PREVIEW_VERSION.

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 72:

> 70:      * <p>This flag is mutually exclusive with {@link #FLAGS_HAS_PREVIEW_VERSION}.
> 71:      */
> 72:     public static final int FLAGS_IS_PREVIEW_VERSION = 0x2;

Seems to be redundant with HAS_PREVIEW_VERSION when seen in a preview location.

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 87:

> 85:      * for {@code /packages/xxx} directories).
> 86:      */
> 87:     public static final int FLAGS_IS_PREVIEW_ONLY = 0x4;

The preview-only case is expected to be the empty set. There seems to be a lot of code dedicated to it.

src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 104:

> 102:     /**
> 103:      * Helper function to calculate preview flags (ATTRIBUTE_PREVIEW_FLAGS).
> 104:      *

A summary of what gets what flags might be easier to read than the code.
Or list the flags and refer to their descriptions (that would describe the paths where the flags would be found)

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 52:

> 50: 
> 51: import static jdk.internal.jimage.ImageLocation.FLAGS_HAS_PREVIEW_VERSION;
> 52: import static jdk.internal.jimage.ImageLocation.FLAGS_IS_PREVIEW_ONLY;

Import utility methods not flags.

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 333:

> 331:                 // it references have that package marked as preview only.
> 332:                 // Skipping these entries avoids empty package subdirectories.
> 333:                 if (previewMode || (flags & FLAGS_IS_PREVIEW_ONLY) == 0) {

Add utility methods for FLAGS_IS_PREVIEW_ONLY == 0

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 336:

> 334:                     pkgDirs.add(ensureCached(newDirectory(pkgDir.getFullName())));
> 335:                 }
> 336:                 if (!previewMode || (flags & FLAGS_HAS_PREVIEW_VERSION) == 0) {

Add utility methods for FLAGS_HAS_PREVIEW_VERSION == 0

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 559:

> 557:             loc = findLocation(name.substring(MODULES_PREFIX.length()));
> 558:             return ensureCachedIfNonNull(
> 559:                     loc != null && loc.getType() == RESOURCE ? newResource(name, loc) : null);

It seems odd to test loc and type to produce a null and them pass it to a method that will do nothing.
The non-productive test can be done and return immediately.

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 66:

> 64:     private static final int FLAGS_HAS_NORMAL_VERSION = 0x2;
> 65:     /** If set, this package exists in preview mode. */
> 66:     private static final int FLAGS_HAS_PREVIEW_VERSION = 0x4;

The flags should be aligned with ImageLocation to avoid confusion. Even if in different contexts the meaning (and bits) should be the same to carry-over the concepts.

The encapsulation of tests for flags helps. It might help in ImageLocation too.

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 86:

> 84:         return new ModuleReference(moduleName, FLAGS_HAS_CONTENT | previewFlag(isPreview));
> 85:     }
> 86: 

Using "of" as the prefix for the method name instead of "for" would align better with many other APIs.

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 122:

> 120:     public boolean hasContent() {
> 121:         return ((flags & FLAGS_HAS_CONTENT) != 0);
> 122:     }

isEmpty could avoid introducing "content" as a loosely defined term.
It would be more similar to directories being empty or lists being empty.

src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 65:

> 63:     FOR_RUNTIME() {
> 64:         @Override
> 65:         boolean resolve() {

Each of these overrides creates an extra subclass.
The alternative is for the resolve method use a switch on the ordinal to return or compute the boolean.

src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystemProvider.java line 125:

> 123:         } else if (envValue instanceof String && Boolean.parseBoolean((String) envValue)) {
> 124:             return PreviewMode.ENABLED;
> 125:         }

This kind of ambiguous type for the environment variable should not be supported. 
Pick a type and stick to it.  
Whereever the jrt: file system is specified will need a description for enablePreview to be added to the spec.

src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java line 79:

> 77: 
> 78:         // TODO: Maybe throw if enablePreview attempted for exploded image?
> 79: 

Exploded image is useful for debugging, it could support preview mode using the same META-INF directory structure.

test/jdk/jdk/internal/jimage/ModuleReferenceTest.java line 228:

> 226: 
> 227:     // Encodes strings sequentially starting from index 100.
> 228:     private static Function<String, Integer> testEncoder() {

The method name `testEncoder` gives the impression of being a test, but it is just a utility function used by a test.

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376687644
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376698440
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376691515
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376692449
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376695695
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376763692
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376761704
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376762324
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2377060646
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376721829
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376723293
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376727666
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2376736057
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2377095899
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2377110493
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2380652378