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

Thu Oct 23 22:34:14 UTC 2025

On Thu, 23 Oct 2025 11:59:02 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   PPC support
>
> src/hotspot/cpu/aarch64/smallRegisterMap_aarch64.inline.hpp line 34:
> 
>> 32: 
>> 33: // Java frames don't have callee saved registers (except for rfp), so we can use a smaller RegisterMap
>> 34: 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.

> src/hotspot/cpu/aarch64/smallRegisterMap_aarch64.inline.hpp line 73:
> 
>> 71:   bool update_map()    const { return false; }
>> 72:   bool walk_cont()     const { return false; }
>> 73:   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.

> src/hotspot/share/interpreter/interpreterRuntime.cpp line 837:
> 
>> 835:         note_trap(current, Deoptimization::Reason_null_check);
>> 836:       }
>> 837:       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).

> src/hotspot/share/interpreter/linkResolver.hpp line 192:
> 
>> 190: };
>> 191: 
>> 192: enum class ClassInitMode : uint8_t {
> 
> If this is just a parameter, does this need to be uint8_t ?

Right, removed.

> src/hotspot/share/oops/instanceKlass.cpp line 810:
> 
>> 808: void InstanceKlass::initialize_preemptable(TRAPS) {
>> 809:   if (this->should_be_initialized()) {
>> 810:     PREEMPT_ON_INIT_SUPPORTED_ONLY(PreemptableInitCall pic(THREAD, this);)
> 
> Are there any other uses of this macro?

I missed to remove this when ppc was added. Removed now.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1123:
> 
>> 1121:       assert(!call.is_valid() || call.is_invokestatic(), "only invokestatic allowed");
>> 1122:       if (call.is_invokestatic() && call.size_of_parameters() > 0) {
>> 1123:         assert(top_frame.interpreter_frame_expression_stack_size() > 0, "should have parameters in exp stack");
> 
> The "this" pointer will be on the top of the interpreter frame expression stack right?  Are size of parameters ever 0 here then?

This is only for `invokestatic` so no this pointer.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1653:
> 
>> 1651:   }
>> 1652:   inline intptr_t* anchor_mark_set_pd();
>> 1653:   inline void anchor_mark_clear_pd();
> 
> The code is really confused whether "pd" is the beginning of the method name or the end.  Both are used but mostly in the beginning of the method. The freeze/thaw code uses it at the end so this is okay.

I added `patch_pd_unused` in 8359222 which should have been `patch_unused_pd`. :)

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1731:
> 
>> 1729:   int bci;
>> 1730:   if (preempt_kind == Continuation::monitorenter) {
>> 1731:     assert(top.is_interpreted_frame() || top.is_runtime_frame(), "");
> 
> So could you use precond() here since it's a precondition and you have no assert message?

Yes, but don’t really see the benefit. It’s replacing a null string for `precond` in a crash.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 2061:
> 
>> 2059: 
>> 2060:   // Only used for preemption on ObjectLocker
>> 2061:   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.

Done.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 2686:
> 
>> 2684: 
>> 2685:   {
>> 2686:     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.

> src/hotspot/share/runtime/javaThread.hpp line 540:
> 
>> 538:     }
>> 539:   };
>> 540: #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.

Done.

> src/hotspot/share/runtime/javaThread.hpp line 1372:
> 
>> 1370: };
>> 1371: 
>> 1372: 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?

> src/hotspot/share/runtime/javaThread.hpp line 1421:
> 
>> 1419: };
>> 1420: 
>> 1421: class ThreadWaitingForClassInit : public StackObj {
> 
> I think this should be in instanceKlass.cpp also near the single caller (unless there's another caller that I don't see).

Done.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457645698
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457646549
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457643620
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457645117
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457648892
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457658418
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457660576
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457662495
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457664724
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457669384
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457670686
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457647838
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2457649861