On Thu, 23 Oct 2025 11:59:02 GMT, Coleen Phillimore <[email protected]> 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
