On Wed, 24 Sep 2025 18:23:16 GMT, Roger Riggs <[email protected]> wrote:
>> 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/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.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1619#discussion_r2413711669
