RFR: 8369238: Allow virtual thread preemption on some common class initialization paths [v3]

[Patricio Chilano Mateo]

On Fri, 17 Oct 2025 05:43:49 GMT, David Holmes <dholmes at openjdk.org> 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