On Thu, 16 Oct 2025 16:13:42 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> If a thread tries to initialize a class that is already being initialized by
>> another thread, it will block until notified. Since at this blocking point
>> there are native frames on the stack, a virtual thread cannot be unmounted
>> and is pinned to its carrier. Besides harming scalability, this can, in some
>> pathological cases, lead to a deadlock, for example, if the thread executing
>> the class initialization method is blocked waiting for some unmounted
>> virtual thread to run, but all carriers are blocked waiting for that class
>> to be initialized.
>>
>> As of JDK-8338383, virtual threads blocked in the VM on `ObjectMonitor`
>> operations can be unmounted. Since synchronization on class initialization
>> is implemented using `ObjectLocker`, we can reuse the same mechanism to
>> unmount virtual threads on these cases too.
>>
>> This patch adds support for unmounting virtual threads on some of the most
>> common class initialization paths, specifically when calling
>> `InterpreterRuntime::_new` (`new` bytecode), and
>> `InterpreterRuntime::resolve_from_cache` for `invokestatic`, `getstatic` or
>> `putstatic` bytecodes. In the future we might consider extending this
>> mechanism to include initialization calls originating from native methods
>> such as `Class.forName0`.
>>
>> ### Summary of implementation
>>
>> The ObjectLocker class was modified to not pin the continuation if we are
>> coming from a preemptable path, which will be the case when calling
>> `InstanceKlass::initialize_impl` from new method
>> `InstanceKlass::initialize_preemptable`. This means that for these cases, a
>> virtual thread can now be unmounted either when contending for the init_lock
>> in the `ObjectLocker` constructor, or in the call to `wait_uninterruptibly`.
>> Also, since the call to initialize a class includes a previous call to
>> `link_class` which also uses `ObjectLocker` to protect concurrent calls from
>> multiple threads, we will allow preemption there too.
>>
>> If preempted, we will throw a pre-allocated exception which will get
>> propagated with the `TRAPS/CHECK` macros all the way back to the VM entry
>> point. The exception will be cleared and on return back to Java the virtual
>> thread will go through the preempt stub and unmount. When running again, at
>> the end of the thaw call we will identify this preemption case and redo the
>> original VM call (either `InterpreterRuntime::_new` or
>> `InterpreterRuntime::resolve_from_cache`).
>>
>> ### Notes
>>
>> `InterpreterRuntime::call_VM_preemptable` used previously only for
>> `InterpreterRuntime::mon...
>
> Patricio Chilano Mateo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix verify_frame_kind
Great piece of work @pchilano ! I've taken an initial pass through the main
class/thread/objectMonitor pieces.
src/hotspot/share/interpreter/linkResolver.cpp line 1122:
> 1120: resolved_klass->initialize(CHECK);
> 1121: } else if (static_mode == StaticMode::initialize_klass_preemptable)
> {
> 1122: resolved_klass->initialize_preemptable(CHECK);
Why is this not CHECK_AND_CLEAR_PREEMPTED?
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`?
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
?
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>`?
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?).
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?
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 196:
> 194: static void do_deopt_after_thaw(JavaThread* thread);
> 195: static bool do_verify_after_thaw(JavaThread* thread, stackChunkOop
> chunk, outputStream* st);
> 196: static void log_frames(JavaThread* thread, bool dolog = true);
Suggestion:
static void log_frames(JavaThread* thread, bool do_log = true);
src/hotspot/share/runtime/javaThread.hpp line 492:
> 490: // throw IE at the end of thawing before returning to Java.
> 491: bool _pending_interrupted_exception;
> 492: // We allow preemption on some klass initializion calls.
Suggestion:
// We allow preemption on some klass initialization calls.
src/hotspot/share/runtime/javaThread.hpp line 507:
> 505:
> 506: bool at_preemptable_init() { return _at_preemptable_init; }
> 507: void set_at_preemptable_init(bool b) { _at_preemptable_init = b; }
If you are going align 503 and 504 then you may as well align these two lines
as well
src/hotspot/share/runtime/javaThread.hpp line 522:
> 520: JavaThread* _thread;
> 521: public:
> 522: AtRedoVMCall(JavaThread *t) : _thread(t) {
Suggestion:
AtRedoVMCall(JavaThread* t) : _thread(t) {
src/hotspot/share/runtime/javaThread.hpp line 524:
> 522: AtRedoVMCall(JavaThread *t) : _thread(t) {
> 523: _thread->_interp_at_preemptable_vmcall_cnt++;
> 524: assert(_thread->_interp_at_preemptable_vmcall_cnt > 0, "");
Suggestion:
assert(_thread->_interp_at_preemptable_vmcall_cnt > 0, "Unexpected count:
%d", _thread->_interp_at_preemptable_vmcall_cnt);
src/hotspot/share/runtime/javaThread.hpp line 528:
> 526: ~AtRedoVMCall() {
> 527: _thread->_interp_at_preemptable_vmcall_cnt--;
> 528: assert(_thread->_interp_at_preemptable_vmcall_cnt >= 0, "");
Suggestion:
assert(_thread->_interp_at_preemptable_vmcall_cnt > 0, "Unexpected count:
%d", _thread->_interp_at_preemptable_vmcall_cnt);
src/hotspot/share/runtime/objectMonitor.cpp line 315:
> 313: }
> 314:
> 315: // Keep object protected during ObjectLocker preemption.
I don't understand why this is necessary.
src/hotspot/share/runtime/synchronizer.cpp line 489:
> 487: if (_thread->preempting()) {
> 488: // If preemption was cancelled we acquired the monitor after
> freezing
> 489: // the frames. Redoing the vm call later in thaw will require us to
I don't quite follow the control flow here - at what point have we frozen any
frames? Was that deep inside `enter`?
src/hotspot/share/runtime/synchronizer.hpp line 230:
> 228: bool _skip_exit;
> 229: public:
> 230: ObjectLocker(Handle obj, TRAPS);
I wonder if we should declare `PREEMPTABLE_TRAPS` as an indicator that the only
exception expected to come out of a call is the preempted-exception?
src/java.base/share/classes/java/lang/VirtualThread.java line 172:
> 170:
> 171: // true when waiting in Object.wait, false for VM internal
> uninterruptible Object.wait
> 172: private volatile boolean interruptableWait;
Suggestion:
private volatile boolean interruptibleWait;
src/java.base/share/classes/java/lang/VirtualThread.java line 605:
> 603: if (s == WAITING || s == TIMED_WAITING) {
> 604: int newState;
> 605: boolean interruptable = interruptableWait;
Suggestion:
boolean interruptible = interruptibleWait;
src/java.base/share/classes/jdk/internal/vm/PreemptedException.java line 31:
> 29: * Internal exception used only by the VM.
> 30: */
> 31: public class PreemptedException extends Exception {
Should probably extend `RuntimeException` so that it is not considered a
checked-exception
-------------
PR Review: https://git.openjdk.org/jdk/pull/27802#pullrequestreview-3348320781
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438580798
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438444689
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438447733
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438469910
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438473899
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438480176
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438482435
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438506706
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438516134
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438518828
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438523339
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438524180
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438533049
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438542039
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438549455
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438568359
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438568876
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2438571309
