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

[Coleen Phillimore]

On Thu, 23 Oct 2025 22:26:57 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

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

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

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

I see, this just verifies if size_of_parameters > 0, there must be something on the expression stack.

>> 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`. :)

I like AnchorMark.  There are other places that clear_anchor in this code.  Can they use AnchorMark?

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

These null strings make me wish we had an assert with no strings if one isn't provided.  I suppose the "precond" string isn't much better. I don't like null strings - it seems like you want to say why you're asserting this condition or what it means, ie take the opportunity to provide a bit more documentation.  Like here you could say that monitorenter is only preempted when the top frame is interpreted or runtime (which is coming from the compiler right?), which I suppose is redundant with the condition.  I suppose nothing is better than "sanity" or "should be".  I retract my suggestion to use precond though.  Others might believe it's better but I'm agnostic.

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

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.

>> 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?

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

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2460563120
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2460632055
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2460637514
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2460668197
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2460687828
PR Review Comment: https://git.openjdk.org/jdk/pull/27802#discussion_r2460751847