8369238: Allow virtual thread preemption on some common class initialization paths #27802

pchilano · 2025-10-14T13:42:18Z

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::monitorenter, was renamed to InterpreterMacroAssembler::call_VM_preemptable_helper and generalized for calls that take more than one argument, and that can return oops and throw exceptions. Method InterpreterMacroAssembler::call_VM_preemptable is now a wrapper that calls the helper, following the pattern of MacroAssembler::call_VM and MacroAssembler::call_VM_helper methods.

As with platform threads, a virtual thread preempted at wait_uninterruptibly that is interrupted will not throw IE, and will preserve the interrupted status. Member _interruptible was added to ObjectWaiter to differentiate this case against Object.wait. Also field interruptableWait was added to VirtualThread class, mainly to avoid an interrupted virtual thread in wait_uninterruptibly to keep looping and submitting the continuation to the scheduler queue until the class is waiting for is initialized.

Currently (and still with this change), when the thread responsible for initializing a class finishes executing the class initializer, it will set the initialization lock to null so the object can be GC'ed. For platform threads blocked waiting on the initialization lock, the Handle in InstanceKlass::initialize_impl will still protect the object from being collected until the last thread exits the monitor. For preempted virtual threads though, that Handle would have already been destroyed. In order to protect the init_lock from being collected while there are still virtual threads using the associated ObjectMonitor, the first preempted virtual thread will put the oop in an OopHandle in the ObjectMonitor (see ObjectMonitor::set_object_strong()), which will be released later when the monitor is deflated.

Preempting at invokestatic means the top frame in the stackChunk can now have the callee’s arguments at the top of the expression stack, which during gc, will need to be processed as part of that frame (no callee yet). Class SmallRegisterMap was therefore modified so that we now have two static instances, one where include_argument_oops() returns true and is used when processing the top frame on this case, and the regular one where it return false and it’s used everywhere else. Also, because InterpretedArgumentOopFinder calculates the address of oops as offsets from the top of the expression stack, we need to correct possible added alignment after the top frame is thawed, since we can safepoint while redoing the VM call. Class AnchorMark was added to deal with this.

Testing

The changes have been running in the Loom pipeline for several months now. They include new test KlassInit.java which exercises preemption on different class initialization cases. Also, the current patch has been run through mach5 tiers 1-8. I'll keep running tests periodically until integration time.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8369238: Allow virtual thread preemption on some common class initialization paths (Enhancement - P3)

Reviewers

David Holmes (@dholmes-ora - Reviewer) 🔄 Re-review required (review applies to c7d6f5c5)
Coleen Phillimore (@coleenp - Reviewer) 🔄 Re-review required (review applies to 13124869)

Contributors

Alan Bateman <alanb@openjdk.org>
Fei Yang <fyang@openjdk.org>
Richard Reingruber <rrich@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27802/head:pull/27802
$ git checkout pull/27802

Update a local copy of the PR:
$ git checkout pull/27802
$ git pull https://git.openjdk.org/jdk.git pull/27802/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27802

View PR using the GUI difftool:
$ git pr show -t 27802

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27802.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-10-14T13:43:56Z

👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-10-14T13:44:40Z

@pchilano This change is no longer ready for integration - check the PR body for details.

openjdk · 2025-10-14T13:45:28Z

@pchilano The following labels will be automatically applied to this pull request:

core-libs
graal
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

openjdk · 2025-10-14T14:28:22Z

@pchilano Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

openjdk · 2025-10-14T14:28:38Z

@pchilano Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

pchilano · 2025-10-14T14:46:31Z

/contributor add @AlanBateman

pchilano · 2025-10-14T14:46:43Z

/contributor add @RealFYang

openjdk · 2025-10-14T14:48:49Z

@pchilano
Contributor Alan Bateman <alanb@openjdk.org> successfully added.

openjdk · 2025-10-14T14:49:50Z

@pchilano
Contributor Fei Yang <fyang@openjdk.org> successfully added.

mlbridge · 2025-10-14T17:18:16Z

Webrevs

TheRealMDoerr · 2025-10-14T19:27:40Z

#27676 has been integrated which requires resolution.

pchilano · 2025-10-14T20:20:55Z

#27676 has been integrated which requires resolution.

Thanks, merged with latest master.

reinrich

Great enhancement indeed @pchilano! The ppc part of it is almost finished. Unfortunately I'm stuck with a problem in verification code already in initial testing. Please see my comment on verify_frame_kind.

reinrich · 2025-10-15T16:54:42Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

+                  RegisterMap::UpdateMap::skip,
+                  RegisterMap::ProcessFrames::skip,
+                  RegisterMap::WalkContinuation::skip);
+      frame fr = top.sender(&reg_map);


I think there's a problem here. I get an assertion on ppc if top is a heap frame (calling from log_preempt_after_freeze) because in frame::sender_raw() we don't take the path we normally would for a frame on heap. Instead sender_for_compiled_frame() is called which uses a constructor that asserts alignment of sp (see here). The assertion fails because _on_heap is false but should be true.

I think in sender_raw map->in_cont() should return true if this frame is on heap.

It's not quite easy to fix though since top can also be on stack.

Right, it should be walked as a heap frame. Could you verify if this patch fixes the issue?

diff --git a/src/hotspot/share/runtime/continuationFreezeThaw.cpp b/src/hotspot/share/runtime/continuationFreezeThaw.cpp index 86c56fe583f..fb1f66c60f4 100644 --- a/src/hotspot/share/runtime/continuationFreezeThaw.cpp +++ b/src/hotspot/share/runtime/continuationFreezeThaw.cpp @@ -196,7 +196,7 @@ static bool do_verify_after_thaw(JavaThread* thread, stackChunkOop chunk, output static void log_frames(JavaThread* thread, bool dolog = true); static void log_frames_after_thaw(JavaThread* thread, ContinuationWrapper& cont, intptr_t* sp); static void print_frame_layout(const frame& f, bool callee_complete, outputStream* st = tty); -static void verify_frame_kind(const frame& top, Continuation::preempt_kind preempt_kind, Method** m_ptr = nullptr, const char** code_name_ptr = nullptr, int* bci_ptr = nullptr); +static void verify_frame_kind(frame& top, Continuation::preempt_kind preempt_kind, Method** m_ptr = nullptr, const char** code_name_ptr = nullptr, int* bci_ptr = nullptr, stackChunkOop chunk = nullptr); #define assert_pfl(p, ...) \ do { \ @@ -1723,7 +1723,7 @@ bool FreezeBase::check_valid_fast_path() { return true; } -static void verify_frame_kind(const frame& top, Continuation::preempt_kind preempt_kind, Method** m_ptr, const char** code_name_ptr, int* bci_ptr) { +static void verify_frame_kind(frame& top, Continuation::preempt_kind preempt_kind, Method** m_ptr, const char** code_name_ptr, int* bci_ptr, stackChunkOop chunk) { Method* m; const char* code_name; int bci; @@ -1747,7 +1747,13 @@ static void verify_frame_kind(const frame& top, Continuation::preempt_kind preem RegisterMap reg_map(current, RegisterMap::UpdateMap::skip, RegisterMap::ProcessFrames::skip, - RegisterMap::WalkContinuation::skip); + RegisterMap::WalkContinuation::include); + if (top.is_heap_frame()) { + assert(chunk != nullptr, ""); + reg_map.set_stack_chunk(chunk); + top = chunk->relativize(top); + top.set_frame_index(0); + } frame fr = top.sender(&reg_map); vframe* vf = vframe::new_vframe(&fr, &reg_map, current); compiledVFrame* cvf = compiledVFrame::cast(vf); @@ -1803,7 +1809,7 @@ static void log_preempt_after_freeze(ContinuationWrapper& cont) { Method* m = nullptr; const char* code_name = nullptr; int bci = InvalidFrameStateBci; - verify_frame_kind(top_frame, pk, &m, &code_name, &bci); + verify_frame_kind(top_frame, pk, &m, &code_name, &bci, cont.tail()); assert(m != nullptr && code_name != nullptr && bci != InvalidFrameStateBci, "should be set"); ResourceMark rm(current);

Your patch fixes the issue. Thanks!

Great, pushed fix.

theRealAph · 2025-10-16T09:05:24Z

src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp

+    // In case the top frame is interpreted we need to set up the anchor using
+    // the last_sp saved in the frame (remove possible alignment added while
+    // thawing, see ThawBase::finish_thaw()). We also need to clear the last_sp
+    // saved in the frame as it is not expected to be set in case we preempt again.


A bit stronger?

Suggested change

// saved in the frame as it is not expected to be set in case we preempt again.

// saved in the frame because it must be clear if we freeze again.

Just to add more context, not clearing last_sp will make this assert [1] fire if we freeze again. That assert is mostly a verification check, because we know the interpreter doesn’t set last_sp for the top frame when calling into the VM. But I don’t see a fundamental reason why it must be cleared (removing the assert and not clearing last_sp works). I don’t see any other code that checks last_sp needs to be cleared for the top frame (other than in the interpreter before calling into the VM).
How about changing that last sentence with: We also clear last_sp to match the behavior when calling the VM from the interpreter (we check for this in FreezeBase::prepare_freeze_interpreted_top_frame).

[1]

jdk/src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp

Line 136 in 87092ef

assert(f.interpreter_frame_last_sp() == nullptr, "should be null for top frame");

FWIW, interpreter_frame_tos_address() behaves differently depending on if last_sp() is cleared or not. I know deoptimization sets last_sp temporarily but makes sure to clear it before giving control back to the interpreter.

Right, but if interpreter_frame_last_sp() is the same as sp() then interpreter_frame_tos_address() will return the same value. My guess is that since we are already setting _last_Java_sp when making VM calls, there is no point in the extra bookkeeping of setting and clearing interpreter_frame_last_sp so we leave it as nullptr.

dholmes-ora

Great piece of work @pchilano ! I've taken an initial pass through the main class/thread/objectMonitor pieces.

dholmes-ora · 2025-10-17T05:43:49Z

src/hotspot/share/interpreter/linkResolver.hpp

  void         print()  PRODUCT_RETURN;
 };

+enum class StaticMode : uint8_t {


Need to think of a better name for this ... ClassInitMode?

Yes, much better. Changed.

dholmes-ora · 2025-10-17T05:45:55Z

src/hotspot/share/interpreter/linkResolver.hpp

+  dont_initialize_klass,
+  initialize_klass,
+  initialize_klass_preemptable


And maybe more concisely:

Suggested change

dont_initialize_klass,

initialize_klass,

initialize_klass_preemptable

dont_init

init

init_preemptable

?

Yes, changed.

dholmes-ora · 2025-10-17T05:58:37Z

src/hotspot/share/oops/instanceKlass.cpp

+  // Block preemption once we are the initializer thread. Unmounting now
+  // would complicate the reentrant case (identity is platform thread).
+  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.

dholmes-ora · 2025-10-17T06:01:15Z

src/hotspot/share/oops/klass.hpp

  virtual bool should_be_initialized() const    { return false; }
  // initializes the klass
  virtual void initialize(TRAPS);
+  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.

Hmmm seems there is an objArrayKlass::initialize (and an empty typeArrayKlass::initialize) but I don't know when such classes would be initialized. I would not expect them to be the target of invokestatic/getstatic/putstatic, nor for "new" but a "new array" would have to do the initialization of the bottom class (at least that is what objArrayKlass::initialize does) - and I don't think the current changes address that case. ??

Anyway leave the placement as-is.

Would an initialize_preemptable() = 0 be better? then you can see if it's called by anything other than InstanceKlass. Klass is always abstract.

We can but we would have to implement it for ArrayKlass too which is kind of the same of what we have now. We have a ShouldNotReachHere() in Klass::initialize_preemptable (following the same pattern as Klass::initialize), so we will catch anything other than InstanceKlass.

I guess following the pattern is better.

dholmes-ora · 2025-10-17T06:04:07Z

src/hotspot/share/runtime/continuationEntry.cpp

+bool ContinuationEntry::assert_entry_frame_laid_out(JavaThread* thread, bool preempted) {
  assert(thread->has_last_Java_frame(), "Wrong place to use this assertion");

+  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.

dholmes-ora · 2025-10-17T06:33:23Z

src/hotspot/share/runtime/synchronizer.hpp

+  bool    _skip_exit;
 public:
-  ObjectLocker(Handle obj, JavaThread* current);
+  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?

Not sure if I like that idea because then we might have to change other callers along the way for this new convention and everybody's already confused by TRAPS so then they'd be confused by a new TRAPS too.

I don't think we would have to. There are a handful of methods that now declare TRAPS but the only exception they should ever encounter is the PreemptException. It would be easier to understand the code if this was evident in their use of TRAPS. Also note it is purely documentation - the definition of PREEMPTABLE_TRAPS is exactly the same as TRAPS

If we want to avoid confusion with other users of ObjectLocker maybe we can leave it as it is and subclass it with a preemptable version? pchilano/jdk@JDK-8369238...pchilano:jdk:PreemptableObjectLocker (this version could also use PREEMPTABLE_TRAPS)

Never mind - I thought there were a few functions that had TRAPS added just for the preempt case but now I don't see them. Thanks

dholmes-ora · 2025-10-17T06:43:30Z

src/java.base/share/classes/java/lang/VirtualThread.java

    private volatile boolean notified;

+    // true when waiting in Object.wait, false for VM internal uninterruptible Object.wait
+    private volatile boolean interruptableWait;


Suggested change

private volatile boolean interruptableWait;

private volatile boolean interruptibleWait;

dholmes-ora · 2025-10-17T06:43:46Z

src/java.base/share/classes/java/lang/VirtualThread.java

        // Object.wait
        if (s == WAITING || s == TIMED_WAITING) {
            int newState;
+            boolean interruptable = interruptableWait;


Suggested change

boolean interruptable = interruptableWait;

boolean interruptible = interruptibleWait;

dholmes-ora · 2025-10-17T06:45:01Z

src/java.base/share/classes/jdk/internal/vm/PreemptedException.java

+/**
+ * Internal exception used only by the VM.
+ */
+public class PreemptedException extends Exception {


Should probably extend RuntimeException so that it is not considered a checked-exception

dholmes-ora · 2025-10-17T06:49:39Z

src/hotspot/share/interpreter/linkResolver.cpp

+    if (static_mode == StaticMode::initialize_klass) {
+      resolved_klass->initialize(CHECK);
+    } else if (static_mode == StaticMode::initialize_klass_preemptable) {
+      resolved_klass->initialize_preemptable(CHECK);


Why is this not CHECK_AND_CLEAR_PREEMPTED?

We need to let the exception propagate all the way back to the VM entry point, and only then we can clear it.

Right - sorry - I though this was the entry point, but it is up in IRT.

reinrich · 2025-10-20T06:30:14Z

@pchilano I've finished the ppc part: reinrich@99b9652

pchilano · 2025-10-20T20:20:33Z

@pchilano I've finished the ppc part: reinrich@99b9652

Great, thanks Richard! Pushed the changes.

pchilano · 2025-10-20T20:20:54Z

/contributor add @reinrich

openjdk · 2025-10-20T20:22:19Z

@pchilano
Contributor Richard Reingruber <rrich@openjdk.org> successfully added.

coleenp

Nice work! This is a first pass with comments. I haven't looked at the platform specific code or tests yet.

coleenp · 2025-10-22T19:17:15Z

src/hotspot/share/interpreter/interpreterRuntime.cpp

        // Recording the trap will help the compiler to potentially recognize this exception as "hot"
        note_trap(current, Deoptimization::Reason_null_check);
      }
+      CLEAR_PENDING_PREEMPTED_EXCEPTION;


You clear this because we want the preempted exception to cause a return to here, but not return an exception to the interpreter to rethrow? Can you add a comment why, especially if I've got this wrong.

You got it right. I realized we can remove this macro which looks odd here, and use CHECK_AND_CLEAR_PREEMPTED at the actual VM entry point as we have for the new case. We only needed to declare resolve_invoke with the TRAPS parameter (as it should have been already). I did the same with resolve_get_put, and also fixed the other methods in resolve_from_cache for consistency. Let me know if you still want a comment about not wanting to throw this exception to Java (not sure where to place it).

coleenp · 2025-10-22T19:19:08Z

src/hotspot/share/interpreter/linkResolver.hpp

  void         print()  PRODUCT_RETURN;
 };

+enum class ClassInitMode : uint8_t {


If this is just a parameter, does this need to be uint8_t ?

Right, removed.

coleenp · 2025-10-23T11:59:02Z

src/hotspot/cpu/aarch64/smallRegisterMap_aarch64.inline.hpp

-  constexpr SmallRegisterMap() = default;
-  ~SmallRegisterMap() = default;
-  NONCOPYABLE(SmallRegisterMap);
+template <bool IncludeArgs>


Can we have a follow-on RFE to make SmallRegisterMap and it's new template in shared code. And only have the platform specific functions here?

Yes, good idea.

coleenp · 2025-10-23T12:00:36Z

src/hotspot/cpu/aarch64/smallRegisterMap_aarch64.inline.hpp

  bool update_map()    const { return false; }
  bool walk_cont()     const { return false; }
-  bool include_argument_oops() const { return false; }
+  bool include_argument_oops() const { return IncludeArgs; }


You made this a template rather than having an _include_argument_oops property for performance?

Not really, using a bool member would work too.

ok, good to know. We could or might not change it in the follow-up issue to move SmallRegisterMap to common code.

coleenp · 2025-10-23T12:04:39Z

src/hotspot/share/runtime/javaThread.hpp

  ~JNIHandleMark() { _thread->pop_jni_handle_block(); }
 };

+class PreemptableInitCall {


If this is only used in instanceKlass.cpp, I think the definition should be there and not in javaThread.hpp. It's not using private methds in javaThread.hpp as far as I can tell.

Moved, and also ThreadWaitingForClassInit. Should I move pre-existent ThreadInClassInitializer too?

It's preexisting so I don't think so. Leave it for a trivial change.

coleenp · 2025-10-23T13:16:17Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

  DEBUG_ONLY(intptr_t* _top_stack_address);

+  // Only used for preemption on ObjectLocker
+  ObjectMonitor* _monitor;


Rather than calling it monitor, you could call it _init_lock so that it makes more sense in the following code. The other reason to give it the same name as in initialization is so in the future we'll see the connection easily.

coleenp · 2025-10-23T13:22:54Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

+  intptr_t* sp = top.sp();
+
+  {
+    HandleMarkCleaner hmc(current);  // Cleanup so._conth Handle


Why doesn't a plain HandleMark do this?
I think you chose HandleMarkCleaner because this is going to back to the interpreter code, so to be like JRT_ENTRY code.
So add something like before going back to the interpreted Java code.

A full HandleMark works too. It’s just that there is already a HandleMark in the callstack (from the original upcall to Java from the carrier thread), so we can use a HandleMarkCleaner here.

The main place we have HandleMarkCleaners are in the JRT_ENTRY. Can you change the comment to:
// Returning to java so cleanup all handles including so._conth Handle

or something like that.

coleenp · 2025-10-23T13:28:06Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

+    log_develop_trace(continuations, preempt)("Redoing InterpreterRuntime::%s for " INT64_FORMAT, code == Bytecodes::Code::_new ? "_new" : "resolve_from_cache", tid);
+
+    // These InterpreterRuntime entry points use JRT_ENTRY which uses a HandleMarkCleaner.
+    // Create a HandeMark to avoid destroying so._conth.


I see. The comment is helpful.

coleenp · 2025-10-23T13:35:00Z

src/hotspot/share/runtime/javaThread.hpp

+             _thread->_interp_at_preemptable_vmcall_cnt);
+    }
+  };
+#endif


Nit: add // ASSERT to the endif.
I always wonder if these big blocks of code added to thread.hpp and javaThread.hpp should be in some new file, and just referenced from this. Not for this change. Just a general comment.

coleenp · 2025-10-23T13:41:16Z

src/hotspot/share/runtime/objectMonitor.cpp

+void ObjectMonitor::set_object_strong() {
+  check_object_context();
+  if (_object_strong.is_empty()) {
+    if (Thread::TrySpinAcquire(&_object_strong_lock)) {


@toxaart Here's a new use of this SpinAcquire/SpinRelease as a TrySpinAcquire.

…AR_PREEMPTED

coleenp

Almost done. Thank you for making suggested changes.

coleenp · 2025-10-24T11:55:28Z

src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp

+  intptr_t* sp = enterSpecial.sp();

-  log_develop_trace(continuations, preempt)("push_cleanup_continuation initial sp: " INTPTR_FORMAT " final sp: " INTPTR_FORMAT, p2i(sp + 2 * frame::metadata_words), p2i(sp));
+  sp[-1] = (intptr_t)StubRoutines::cont_preempt_stub();


push_cleanup_continuation sets sp[-2]. This doesn't have to set that?

push_cleanup_continuation() doesn’t need it. I removed it there and added a comment on both methods.

coleenp · 2025-10-24T13:46:06Z

src/hotspot/cpu/aarch64/smallRegisterMap_aarch64.inline.hpp

  bool update_map()    const { return false; }
  bool walk_cont()     const { return false; }
-  bool include_argument_oops() const { return false; }
+  bool include_argument_oops() const { return IncludeArgs; }


ok, good to know. We could or might not change it in the follow-up issue to move SmallRegisterMap to common code.

coleenp · 2025-10-24T13:48:33Z

src/hotspot/cpu/x86/continuationFreezeThaw_x86.inline.hpp

+    _top_frame.interpreter_frame_set_last_sp(_last_sp_from_frame);
+    intptr_t* sp = _top_frame.sp();
+    if (sp != _last_sp_from_frame) {
+      sp[-1] = (intptr_t)_top_frame.pc();


Same coment as aarch64, does this need to set sp[-2] to fp like above? Or should it preserve the value? Can you add a comment for each telling why?

Here we need to set it because it’s the value that will be set as _last_Java_fp in the _anchor. Added a comment.

coleenp · 2025-10-24T13:48:56Z

src/hotspot/cpu/x86/continuationFreezeThaw_x86.inline.hpp

+  intptr_t* sp = enterSpecial.sp();

-  log_develop_trace(continuations, preempt)("push_cleanup_continuation initial sp: " INTPTR_FORMAT " final sp: " INTPTR_FORMAT, p2i(sp + 2 * frame::metadata_words), p2i(sp));
+  sp[-1] = (intptr_t)StubRoutines::cont_preempt_stub();


Same here sp-2 ?

coleenp · 2025-10-24T13:58:07Z

src/hotspot/cpu/aarch64/stackChunkFrameStream_aarch64.inline.hpp

  ResourceMark rm;
-  InterpreterOopMap mask;
  frame f = to_frame();
-  f.interpreted_frame_oop_map(&mask);


There are two uses of this function left in continuationHelper.inline.hpp and continuationFreezeThaw.cpp under verification code. Maybe they can be removed? Do the places that call this in verification code still valid for preempted class initialization? Do they need to count arguments now?

The verification code in verify_frame_top is still valid. Technically we should count arguments if f is the top frame when this is a preemption on invokestatic case, and assert that the result equals top. But that would overcomplicate the code for not much gain. The other caller is ContinuationHelper::Frame::frame_top(const frame &f), which I see is only called from ThawBase::recurse_thaw_interpreted_frame. It is also still valid and is never called for the top frame, so no argument count is needed.

coleenp · 2025-10-24T14:09:36Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

+  intptr_t* sp = top.sp();
+
+  {
+    HandleMarkCleaner hmc(current);  // Cleanup so._conth Handle


The main place we have HandleMarkCleaners are in the JRT_ENTRY. Can you change the comment to:
// Returning to java so cleanup all handles including so._conth Handle

or something like that.

coleenp · 2025-10-24T14:11:40Z

src/hotspot/share/runtime/frame.cpp

+      //       fore handling the exception (the exception handling
+      //       code in the interpreter calls a blocking runtime
+      //       routine which can cause this code to be executed).
+      //       (was bug gri 7/27/98)


I like the refactoring of this condition. It may be finally time to remove this line from 1998. I, for one, will miss it but it doesn't really help anyone with anything.

Ok, I removed that part of the comment.

coleenp · 2025-10-24T14:16:56Z

test/jdk/java/lang/Thread/virtual/KlassInit.java

+ * @test id=gc
+ * @library /test/lib
+ * @requires vm.debug == true & vm.continuations
+ * @run junit/othervm/timeout=480 -XX:+UnlockDiagnosticVMOptions -XX:+FullGCALot -XX:FullGCALotInterval=1000 -XX:CompileCommand=exclude,KlassInit::lambda$testReleaseAtKlassInit* -XX:CompileCommand=exclude,KlassInit$$Lambda*::run -XX:CompileCommand=exclude,KlassInit$1Driver::foo KlassInit


I think you can use multiple lines for the run command and not have to wrap like this.

coleenp · 2025-10-24T14:19:44Z

src/hotspot/share/runtime/synchronizer.hpp

+  bool    _skip_exit;
 public:
-  ObjectLocker(Handle obj, JavaThread* current);
+  ObjectLocker(Handle obj, TRAPS);


Not sure if I like that idea because then we might have to change other callers along the way for this new convention and everybody's already confused by TRAPS so then they'd be confused by a new TRAPS too.

coleenp · 2025-10-24T14:22:21Z

src/hotspot/share/oops/klass.hpp

  virtual bool should_be_initialized() const    { return false; }
  // initializes the klass
  virtual void initialize(TRAPS);
+  virtual void initialize_preemptable(TRAPS);


I guess following the pattern is better.

coleenp

I had a very small comment that I noticed, but this looks great. Excellent work solving this class initialization problem for virtual threads.

coleenp · 2025-10-27T21:20:39Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

+  }
+}
+
+static void log_preempt_after_freeze(ContinuationWrapper& cont) {


Does this modify ContinuationWrapper? I don't see how it does. If not, it should be a const reference.

coleenp · 2025-10-27T21:22:51Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

+  return sp;
+}
+
+intptr_t* ThawBase::redo_vmcall(JavaThread* current, frame& top) {


Does this modify "top"? Else should be a const reference too. Looks like a lot of references are non-const. If these methods don't modify their non-const reference parameters, I think you should have a cleanup pass to fix these to be const references.

It’s used in AnchorMark to set _top_frame, which can’t be const because of the interpreter_frame_set_last_sp usage. I fixed push_return_frame too, let me know if you spotted other ones.

pchilano · 2025-10-28T19:34:08Z

Thanks for the review Coleen!

RealFYang · 2025-10-29T01:45:25Z

Just FYI: I am witnessing build issues after applying this on JDK HEAD. So we might want to merge with latest master. @pchilano

dholmes-ora

A few additional comments/suggestions, but overall this looks good (to the extent I understand the details). Thanks.

dholmes-ora · 2025-10-29T06:00:40Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp

  ContinuationWrapper::SafepointOp so(current, _cont);
-  // Since we might safepoint set the anchor so that the stack can be walked.
-  set_anchor(current, top.sp());
+  AnchorMark am(current, top);  // Set the anchor so that the stack is walkable.


Shouldn't you delete the clear_anchor at line 2739 below?

Yes, good catch. Removed.

dholmes-ora · 2025-10-29T06:02:57Z

src/hotspot/share/runtime/javaCalls.cpp

  guarantee(thread->is_Java_thread(), "crucial check - the VM thread cannot and must not escape to Java code");
  assert(!thread->owns_locks(), "must release all locks when leaving VM");
  guarantee(thread->can_call_java(), "cannot make java calls from the native compiler");
+  assert(!thread->preempting(), "");


I'm not sure why this is checked here, and there is no error message to tell me. If we did get here with preempting set what would that mean?

This is a safety check since a thread marked as preempted should not be making upcalls to Java. It should be bailing out from methods and returning to the VM entry point. I found we could get here from the exception path (from your other comment below) when there was no NoPreemptMark there.

Okay then how about "Unexpected Java upcall whilst processing preemption" ?

dholmes-ora · 2025-10-29T06:11:56Z

src/hotspot/share/runtime/synchronizer.hpp

+  bool    _skip_exit;
 public:
-  ObjectLocker(Handle obj, JavaThread* current);
+  ObjectLocker(Handle obj, TRAPS);


I don't think we would have to. There are a handful of methods that now declare TRAPS but the only exception they should ever encounter is the PreemptException. It would be easier to understand the code if this was evident in their use of TRAPS. Also note it is purely documentation - the definition of PREEMPTABLE_TRAPS is exactly the same as TRAPS

dholmes-ora · 2025-10-29T06:13:56Z

src/hotspot/share/runtime/thread.cpp

+bool Thread::TrySpinAcquire(volatile int * adr) {
+  return AtomicAccess::cmpxchg(adr, 0, 1) == 0;


How is this a try-spin-acquire operation ??? I don't think we need this, we can just inline the cmpxchg where needed.

Right, not sure why I used that class. I replaced it with the atomic operations.

dholmes-ora · 2025-10-29T06:19:07Z

src/hotspot/share/utilities/exceptions.cpp

+    // We could get here while linking or initializing a klass
+    // from a preemptable call. Don't preempt here since before
+    // the exception is propagated we might make an upcall to
+    // Java to initialize the object with the cause of exception.
+    NoPreemptMark npm(thread);


Could you explain the control flow in more detail here please. I'm unclear both how we get here and exactly what the affect of the NoPreemptMark is.

We can get here from a preemptable path if initialization of the klass failed: https://github.com/pchilano/jdk/blob/c7d6f5c5220a93653dea37488d238a76e2ad627d/src/hotspot/share/oops/instanceKlass.cpp#L1292
Also from here at linking step: https://github.com/pchilano/jdk/blob/c7d6f5c5220a93653dea37488d238a76e2ad627d/src/hotspot/share/oops/instanceKlass.cpp#L970.
The klass of the exception might need to be initialized, so without this NoPreemptMark the thread could be preempted while trying to initialize it. The problem is that this method is called here https://github.com/pchilano/jdk/blob/c7d6f5c5220a93653dea37488d238a76e2ad627d/src/hotspot/share/utilities/exceptions.cpp#L372, which will continue executing and possibly make an upcall to Java. We could potentially change these methods to identify a PreemptedException and use the CHECK macros to return, but I think it is simpler to disable preemption for these cases.

I'm a correct that at line 348 "the exception is propagated we might make an upcall to" you are referring to an PreemptedException? You could change the comment to distinguish this one with the exception being created better.

Yes, I updated the comment to make the distinction clearer.

pchilano · 2025-10-29T20:49:00Z

Just FYI: I am witnessing build issues after applying this on JDK HEAD. So we might want to merge with latest master. @pchilano

Thanks, merged with latest master.

dholmes-ora

Thanks for explanations and updates.

dholmes-ora

Nominal approval but obviously you need additional approvers for this one.

pchilano · 2025-10-30T15:50:30Z

Thanks for the review David!

v1

60cbca4

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 14, 2025

pchilano added 2 commits October 14, 2025 10:07

Fix whitespaces

88844bf

RISC-V support

d035366

pchilano marked this pull request as ready for review October 14, 2025 17:11

openjdk bot added the rfr Pull request is ready for review label Oct 14, 2025

Merge branch 'master' into JDK-8369238

79fce80

reinrich reviewed Oct 15, 2025

View reviewed changes

theRealAph reviewed Oct 16, 2025

View reviewed changes

fix verify_frame_kind

ac55715

dholmes-ora reviewed Oct 17, 2025

View reviewed changes

pchilano added 4 commits October 20, 2025 14:22

Use ClassInitMode and shorter enum names

9f4436a

Extra comments from David

30bdf49

Remove extra debugging param

6882db0

PPC support

6000edb

coleenp reviewed Oct 23, 2025

View reviewed changes

pchilano added 3 commits October 23, 2025 17:28

define methods in resolve_from_cache with TRAPS and use CHECK_AND_CLE…

a943c83

…AR_PREEMPTED

Extra comments from Coleen

0e483ba

rename _monitor to _init_lock

2786b1d

coleenp reviewed Oct 24, 2025

View reviewed changes

pchilano added 2 commits October 27, 2025 15:20

More comments from Coleen

bea5620

Improve comment in anchor_mark_set_pd

1312486

coleenp approved these changes Oct 27, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 27, 2025

add const to references

3bf8ebd

openjdk bot removed the ready Pull request is ready to be integrated label Oct 28, 2025

dholmes-ora reviewed Oct 29, 2025

View reviewed changes

pchilano added 2 commits October 29, 2025 14:59

Merge branch 'master' into JDK-8369238

4cecd7e

More fixes from David's comments

c7d6f5c

dholmes-ora reviewed Oct 30, 2025

View reviewed changes

dholmes-ora approved these changes Oct 30, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2025

Improve comment and assert msg

ffcd92a

openjdk bot removed the ready Pull request is ready to be integrated label Oct 30, 2025

	// saved in the frame as it is not expected to be set in case we preempt again.
	// saved in the frame because it must be clear if we freeze again.

	private volatile boolean interruptableWait;
	private volatile boolean interruptibleWait;

	boolean interruptable = interruptableWait;
	boolean interruptible = interruptibleWait;

		bool Thread::TrySpinAcquire(volatile int * adr) {
		return AtomicAccess::cmpxchg(adr, 0, 1) == 0;

8369238: Allow virtual thread preemption on some common class initialization paths #27802

Are you sure you want to change the base?

8369238: Allow virtual thread preemption on some common class initialization paths #27802

Conversation

pchilano commented Oct 14, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of implementation

Notes

Testing

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

pchilano commented Oct 14, 2025

Uh oh!

pchilano commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

mlbridge bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TheRealMDoerr commented Oct 14, 2025

Uh oh!

pchilano commented Oct 14, 2025

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pchilano commented Oct 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 14, 2025 •

edited

Loading

openjdk bot commented Oct 14, 2025 •

edited

Loading

mlbridge bot commented Oct 14, 2025 •

edited

Loading