On Fri, 17 Oct 2025 05:43:49 GMT, David Holmes <[email protected]> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fix verify_frame_kind
>
> src/hotspot/share/interpreter/linkResolver.hpp line 192:
>
>> 190: };
>> 191:
>> 192: enum class StaticMode : uint8_t {
>
> Need to think of a better name for this ... `ClassInitMode`?
Yes, much better. Changed.
> src/hotspot/share/interpreter/linkResolver.hpp line 195:
>
>> 193: dont_initialize_klass,
>> 194: initialize_klass,
>> 195: initialize_klass_preemptable
>
> And maybe more concisely:
> Suggestion:
>
> dont_init
> init
> init_preemptable
>
> ?
Yes, changed.
> src/hotspot/share/oops/instanceKlass.cpp line 1284:
>
>> 1282: // Block preemption once we are the initializer thread. Unmounting
>> now
>> 1283: // would complicate the reentrant case (identity is platform thread).
>> 1284: NoPreemptMark npm(THREAD);
>
> How does this affect unrelated "preemption" that may occur in the Java code
> called as part of `<clinit>`?
While executing the `<clinit>` method the vthread is pinned because of the
call_stub in the stack (freeze will fail if we try to unmount), regardless of
this `NoPreemptMark`. So this is for the other places where it is preemptable,
mainly when initializing the super klass below.
> src/hotspot/share/oops/klass.hpp line 583:
>
>> 581: // initializes the klass
>> 582: virtual void initialize(TRAPS);
>> 583: virtual void initialize_preemptable(TRAPS);
>
> Can we not define these on `instanceKlass` instead of `klass`? Seems really
> odd to declare virtual methods for which the base class version should never
> be called (they should be pure virtuals in that case I would think?).
Ok. Just to double check, casting to `InstanceKlass*` where we call
`initialize_preemptable` for the `invokestatic` and `getstatic/putstatic` cases
should be always safe right? I don’t see how we could get there for an
`ArrayKlass`.
> src/hotspot/share/runtime/continuationEntry.cpp line 117:
>
>> 115: assert(thread->has_last_Java_frame(), "Wrong place to use this
>> assertion");
>> 116:
>> 117: if (preempted) return true;
>
> I don't see the point of adding a new parameter just so the method can check
> it and return immediately. Shouldn't the caller be checking this?
Right, fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2445862480
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2445862813
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2445864285
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2445868222
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2445869014
