On Wed, 8 Oct 2025 12:35:21 GMT, David Beaumont <[email protected]> wrote:
>> 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.
>
> Well it's a small set, but vitally important. Preview only is the case where
> you can't lookup the normal resource first and check its flags to see if
> there's a preview version. Since you must correct find all preview-only
> resources, you are left with either:
>
> * *always* speculatively looking up all resources in their preview location
> when not found otherwise.
> * Doing something "cleverer" to reduce double lookups.
>
> In the C++ class-loader (as I understand it) most lookups will be for things
> referenced in other class files, and thus expected to exist. This makes "if
> you can't find the normal version, look for the preview version" acceptable
> (I think) because this should be only in cases where you're looking for
> something that should exist at one of the two paths.
>
> In the Java stuff, there's a lot of speculative lookup, especially for
> non-class resources. All modules are tested for any non-existent resource. So
> in this case the strategy needs to be cleverer or you'll basically (at least)
> double the cost of a lot of things.
>
> So I settled on the easiest approach to be to just (efficiently) pre-scan for
> preview resources and put them in the node cache during init (this is only
> about the same work the old code did for link nodes etc.). Once they're
> cached, the lookup code no longer has to care about 2nd lookups or anything.
> Preview versions and preview-only versions are just treated the same, they're
> in the cache, so they're found and returned without looking at anything else.
>
> This is partly why there's the "ensureCached" method which guarantees you
> never replace what's in the cache with anything else (so preview stuff put
> there during init of the reader is "safe" from accidental overwriting).
>
> The new flag stuff and change of ordering of package entries is there mostly
> to support efficient pre-scanning of the small set of preview only resources.
I'm still going to say, there are no plans to have any Preview-only resources.
And yes, a preview-only flag would be useful to (mostly) avoid a second lookup.
If I understand the design, there is still a location in the normal tree with
the preview-only flag but there is no resource at that location, only the
flags. And a second lookup is needed to find the location in the preview
heirarchy. The caching avoids repeated 2nd lookups.
>> 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.
>
> Other file-system flags already do this, so I was just copying that approach.
> Happy to not do this though.
> Which do you suggest, Boolean or String?
Strings are more frequent and conventional.
>> 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.
>
> Indeed. I can support it later, I'm just wondering if, in this partially
> complete state, I should complain or not.
> I've got most of the code for that done somewhere or other.
Throwing with a message would avoid doubt about whether it is supported.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1619#discussion_r2414290560
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1619#discussion_r2414296625
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1619#discussion_r2414298186
