On Mon, 30 Mar 2026 16:15:07 GMT, David Beaumont <[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). >> >> --------- >> - [ ] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > David Beaumont has updated the pull request incrementally with two additional > commits since the last revision: > > - Rename ModuleReference to ModuleLink > - tweak comments This is a large patch and it takes me quite some time to get through it in full. Lots of tricky edge cases. This is an initial round of a review. From my study of the code this would break `jlink` on a JEP 493 enabled JDK (e.g. it would get inconsistent results depending on whether `--enable-preview` is on or not). `ModuleContentSpliterator` would need a change for this too. I'd also suggest to expand on the `test/jdk/java/lang/module/ModuleReader/ModuleReaderTest.java` test with a module that contains preview classes to cover some basic cases. More comments inline. Thanks! src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 38: > 36: * to the jimage file provided by the shipped JDK by tools running on JDK > 8. > 37: * */ > 38: public enum PreviewMode { Some users of this enum would be in need of a fourth option: `NONE`. A mode that would return the contents of the `jimage` verbatim, preview or not. I wonder if `PreviewMode` is the right name. src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystem.java line 90: > 88: this.provider = provider; > 89: // TODO: Obtain and pass correct preview mode flag value here. > 90: this.image = SystemImage.open(PreviewMode.DISABLED); // open > image file I believe this breaks `ModuleReader.list()` [spec](https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/lang/module/ModuleReader.html#list()): Lists the contents of the module, returning a stream of elements that are the names of all resources in the module. Whether the stream of elements includes names corresponding to directories in the module is module reader specific. src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 395: > 393: */ > 394: private static class SystemImage { > 395: static final ImageReader READER = SystemImageReader.get(); `SystemImage.reader()` is internally used for `ModuleReader.list()` which probably need special casing to list all resources. For example, it would **not** `list()` the resource, but `mReader.open("META-INF/preview/java/lang/Byte.class")` would return an (optional) `InputStream` an inconsistency. src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 51: > 49: > 50: // ImageReader to access resources in jimage. > 51: private static final ImageReader READER = SystemImageReader.get(); This will use `ImageReader.open(..., PreviewMode.FOR_RUNTIME)` which is likely not what it's intended for. Just a wrapper around resources in the runtime image. e.g. `lib/modules`. It should use the `NONE` mode instead here (give me all resources in the modules image). src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 107: > 105: .orElseThrow(() -> new IllegalArgumentException( > 106: "Module " + module + " not part of the JDK > install")); > 107: this.imageResources = SystemImageReader.getResourceEntries(); `SystemImageReader.getResourceEntries()` uses `ImageReader.open(..., PreviewMode.FOR_RUNTIME)` which doesn't necessarily list **all** resources in a runtime image. E.g. without `--enable-preview` it lists a different set of resources from the runtime image than without it. The use-case here is to get **all** resources in a module, preview class or not. ------------- PR Review: https://git.openjdk.org/jdk/pull/29414#pullrequestreview-4129196218 PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r3100757260 PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r3100656253 PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r3100795391 PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r3100767594 PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r3100736285
