RFR: 8368467: [lworld] Add new flag generation for jimage to support preview mode [v9]

Mon Oct 27 15:43:12 UTC 2025

On Mon, 27 Oct 2025 12:55:36 GMT, David Beaumont <duke at openjdk.org> 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.
>> 
>> Compare and review this against https://github.com/openjdk/valhalla/pull/1619.
>
> David Beaumont has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
> 
>   Rollup after resync of dependent PR.
>   
>   * Fixing up after dependent PR changes
>   * feedback and remove unused code
>   * new tests for ImageLocation
>   * Restoring lost changes and updating some comments.
>   * add system property guard to preview mode
>   * Remove TODOs now jimage version is bumped
>   * jimage writer changes to support preview mode.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java line 206:

> 204:                 } catch (InvalidTreeException err) {
> 205:                     // It has been observed some badly created jar file to contain
> 206:                     // invalid directory entry marked as not directory (see 8131762).

I'm not 100% sure of the value of retaining stderr logging like this, especially if the code continues after ignoring the input, but it's the same behaviour as before, so I'm leaving it as-is.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java line 261:

> 259:             // Intermediate packages are marked "empty" (no resources). This might
> 260:             // later be merged with a non-empty reference for the same package.
> 261:             ModuleReference emptyRef = ModuleReference.forEmptyPackageIn(modName, isPreviewPath);

This convinces me that `forEmptyPackageIn` is a better name than `forEmptyPackage`, since the passing of the module name as the first parameter no longer raises eyebrows.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java line 276:

> 274:                 ModuleReference resourceRef = ModuleReference.forPackageIn(modName, isPreviewPath);
> 275:                 packageToModules.computeIfAbsent(fullPkgName, p -> new HashSet<>()).add(resourceRef);
> 276:                 // Init adds new node to parent (don't add resources to directAccess).

I'd be happy to rename the `directAccess` field. It's really a holder for directories. The fact it's "direct access" is rather a statement of its form (obvious since it's a map really) and not its function.
Thoughts?

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java line 295:

> 293:         }
> 294: 
> 295:         // Helper to iterate package names up to, and including, the complete name.

If this comment isn't clear enough I can elaborate. Basically lets you iterate indices in "foo.bar.baz" as `3, 7, 11, -1` (including an index at the end of the string before it returns `-1`). This is just here to make the for-loop cleaner where the real business logic is.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java line 311:

> 309:         }
> 310: 
> 311:         private String removeRadical(Node node) {

I don't think this method obviously earns its keep, so would be happy to inline it, perhaps with a string constant. But also happy to leave it.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java line 416:

> 414:                         // A resource location, remove "/modules"
> 415:                         String s = tree.toResourceName(current);
> 416:                         current.setLocation(outLocations.get(s));

Calling this via the method adds a check that nothing is being overwritten.

test/jdk/jdk/internal/jimage/ImageLocationTest.java line 89:

> 87:     }
> 88: 
> 89:     @Test

The `getPackageFlags` tests check that module references get merged correctly to create the parent directory flags we expect. While adding individual resources you build up a *list* of references in each package, one for each resource (directly or indirectly) in that package.
This is a key reason for how the new code can simplify things around the outer "add each resource in turn" loop, by just collecting the references and merging them later to get the right result.

test/jdk/jdk/internal/jimage/ImageLocationTest.java line 119:

> 117:                 ModuleReference.forEmptyPackage("modbaz", true));
> 118:         int previewOnlyFlags = ImageLocation.getPackageFlags(refs);
> 119:         // Note the asymmetry between this and the getFlags() case. Unlike

This is a flag combination you can't see for locations in `/modules`, because "has-preview-version" there means "is-not-a-preview-resource" and thus cannot imply "preview only". But there is no concept of "is-a-preview-resource" for things in the `/packages` space.

However, the code using the flags doesn't really care about what combinations exist since the flags are designed to be used independently of each other, each answering a specific question at a certain place in the processing code.
* `hasPreviewVersion` means:
    *  Put this entry first so we can skip 99% of packages with no preview content during pre-processing.
* `isPreviewOnly` means:
    *  Don't add this to the `/packages` directory when *not* in preview mode.
    *  Put the node for this location in the preview-only map to be added separately during directory completion (in preview mode).

test/jdk/jdk/internal/jimage/ImageReaderTest.java line 251:

> 249: 
> 250:             // Preview version of classes either overwrite existing entries or are added to directories.
> 251:             assertEquals("Preview: com.foo.HasPreviewVersion", loader.loadAndGetToString("modfoo", "com.foo.HasPreviewVersion"));

This "load the class, run its `toString()` method and test the result" could be encapsulated a bit more if you think it would make the intent of the test clearer. E.g.:

assertClassToString("modfoo", "com.foo.HasPreviewVersion", "Preview: com.foo.HasPreviewVersion", loader);

Or even:

loader.assertNormalVersion("modfoo", "com.foo.NormalFoo");
loader.assertPreviewVersion("modfoo", "com.foo.HasPreviewVersion");

Thoughts?

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

Sorry about this enormous non-diff. It's because of moving the static init into a holder class because we now want "module" + "path" as two separate things, pre-calculated, for benchmarking the new APIs.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465642516
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465648972
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465656652
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465667236
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465672206
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465677574
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465700949
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465738045
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465769498
PR Review Comment: https://git.openjdk.org/valhalla/pull/1621#discussion_r2465776447