On Fri, 13 Mar 2026 06:13:27 GMT, David Holmes <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> More feedback tweaks
>
> src/hotspot/share/classfile/classLoader.cpp line 277:
>
>> 275: // jimage_is_preview_enabled() can be called to correctly determine the
>> access mode.
>> 276: static bool jimage_is_initialized() {
>> 277: return jimage_exists() && Preview_mode != PREVIEW_MODE_UNINITIALIZED;
>
> If the JImage doesn't exist then presumably the Preview_mode will never
> change from PREVIEW_MODE_UNINITIALIZED, so that is all we need to check here.
True ... I think. I'll double check and change it if so (maybe with a comment).
> src/hotspot/share/classfile/classLoader.cpp line 1474:
>
>> 1472: // Finishes initializing the JImageFile (if present) by setting the
>> access mode.
>> 1473: void ClassLoader::set_preview_mode(bool enable_preview) {
>> 1474: assert(Preview_mode == PREVIEW_MODE_UNINITIALIZED, "set_preview_mode
>> must not be called twice");
>
> So should this check, or assert, that the JImageFile exists?
Hmmm. It's a code flow assertion making sure this is called at most once, and
only after init.
If the jimage file didn't exist, we should never get to the point where this is
called (but that's assuming it's called in the right place I guess).
So if this were called too early it would fail with a misleading error message
I guess. It would never pass spuriously though. I think it's okay as it is, but
I'll change it if you want.
> src/hotspot/share/classfile/classLoader.hpp line 364:
>
>> 362: static char* lookup_vm_options();
>> 363:
>> 364: // Called once, after all flags are processed, to finish initializing
>> the
>
> This method doesn't "finish initializing the JImage file" it just marks it as
> initialized.
Yes. The jimage file initialization flag/check is to prevent people looking for
resources too early.
The root issue here is that preview mode (which affects what's returned from
jimage) can be affected by configuration *in* jimage. So we must do a "raw"
read on jimage *before* we know if preview mode is set, but we also want to
prevent accidentally reading anything else with the wrong flag.
The "finishing initialization" is just "marking it as initialized" because
we've finally determined if preview mode is set or not.
Really this is an artifact of not using a new object/structure to encapsulate
"jimage plus preview mode flag" since the jimage on its own is usable as soon
as we open it, and must be used one its own to read the config, but we're also
layering on an additional requirement of always using the correct preview mode
flag with it for anything else.
Thus there are two, slightly distinct, life-cycles here, which is why I wanted
to pull this "jimage plus flag" thing out via these helper methods.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2930259846
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2930262625
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2930308469
