On Wed, 30 Oct 2024 12:48:02 GMT, Fredrik Bredberg <[email protected]>
wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Rename set/has_owner_anonymous to set/has_anonymous_owner
>> - Fix comments in javaThread.hpp and Thread.java
>> - Rename nonce/nounce to seqNo in VirtualThread class
>> - Remove ObjectMonitor::set_owner_from_BasicLock()
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp line 945:
>
>> 943:
>> 944: void inc_held_monitor_count();
>> 945: void dec_held_monitor_count();
>
> I prefer to pass the `tmp` register as it's done in PPC. Manual register
> allocation is hard as it is, hiding what registers are clobbered makes it
> even harder.
>
> Suggestion:
>
> void inc_held_monitor_count(Register tmp);
> void dec_held_monitor_count(Register tmp);
Changed.
> src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 740:
>
>> 738: void MacroAssembler::clobber_nonvolatile_registers() {
>> 739: BLOCK_COMMENT("clobber nonvolatile registers {");
>> 740: Register regs[] = {
>
> Maybe I've worked in the embedded world for too, but it's always faster and
> safer to store arrays with values that never change in read only memory.
> Suggestion:
>
> static const Register regs[] = {
Added.
> src/hotspot/cpu/riscv/continuationFreezeThaw_riscv.inline.hpp line 273:
>
>> 271: ? frame_sp + fsize - frame::sender_sp_offset
>> 272: // we need to re-read fp because it may be an oop and we might
>> have fixed the frame.
>> 273: : *(intptr_t**)(hf.sp() - 2);
>
> Suggestion:
>
> : *(intptr_t**)(hf.sp() - frame::sender_sp_offset);
Changed.
> src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 793:
>
>> 791:
>> 792: void inc_held_monitor_count(Register tmp = t0);
>> 793: void dec_held_monitor_count(Register tmp = t0);
>
> I prefer if we don't use any default argument. Manual register allocation is
> hard as it is, hiding what registers are clobbered makes it even harder. Also
> it would make it more in line with how it's done in PPC.
> Suggestion:
>
> void inc_held_monitor_count(Register tmp);
> void dec_held_monitor_count(Register tmp);
Changed.
> src/hotspot/share/runtime/continuation.cpp line 125:
>
>> 123: };
>> 124:
>> 125: static bool is_safe_vthread_to_preempt_for_jvmti(JavaThread* target,
>> oop vthread) {
>
> I think the code reads better if you change to
> `is_safe_to_preempt_vthread_for_jvmti`.
> Suggestion:
>
> static bool is_safe_to_preempt_vthread_for_jvmti(JavaThread* target, oop
> vthread) {
I renamed it to is_vthread_safe_to_preempt_for_jvmti.
> src/hotspot/share/runtime/continuation.cpp line 135:
>
>> 133: #endif // INCLUDE_JVMTI
>> 134:
>> 135: static bool is_safe_vthread_to_preempt(JavaThread* target, oop vthread)
>> {
>
> I think the code reads better if you change to `is_safe_to_preempt_vthread`.
> Suggestion:
>
> static bool is_safe_to_preempt_vthread(JavaThread* target, oop vthread) {
I renamed it to is_vthread_safe_to_preempt, which I think it reads even better.
> src/hotspot/share/runtime/continuation.hpp line 66:
>
>> 64:
>> 65: enum preempt_kind {
>> 66: freeze_on_monitorenter = 1,
>
> Is there a reason why the first enumerator doesn't start at zero?
There was one value that meant to be for the regular freeze from java. But it
was not used so I removed it.
> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 889:
>
>> 887: return f.is_native_frame() ? recurse_freeze_native_frame(f, caller)
>> : recurse_freeze_stub_frame(f, caller);
>> 888: } else {
>> 889: return freeze_pinned_native;
>
> Can you add a comment about why you only end up here for
> `freeze_pinned_native`, cause that is not clear to me.
We just found a frame that can't be freezed, most likely the call_stub or
upcall_stub which indicate there are further natives frames up the stack. I
added a comment.
> src/hotspot/share/runtime/objectMonitor.cpp line 1193:
>
>> 1191: }
>> 1192:
>> 1193: assert(node->TState == ObjectWaiter::TS_ENTER || node->TState ==
>> ObjectWaiter::TS_CXQ, "");
>
> In `ObjectMonitor::resume_operation()` the exact same line is a `guarantee`-
> not an `assert`-line, is there any reason why?
The `guarantee` tries to mimic the one here:
https://github.com/openjdk/jdk/blob/ae82cc1ba101f6c566278f79a2e94bd1d1dd9efe/src/hotspot/share/runtime/objectMonitor.cpp#L1613
The assert at the epilogue is probably redundant. Also in `UnlinkAfterAcquire`,
the else branch already asserts `ObjectWaiter::TS_CXQ`. I removed it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825101744
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825108078
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825100526
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825101246
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825107036
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825102359
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825103008
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825104666
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825106368
