On Thu, 20 Nov 2025 22:17:53 GMT, David Holmes <[email protected]> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add Alan's comment in VirtualThread > > src/hotspot/share/classfile/javaClasses.cpp line 1757: > >> 1755: jint* addr = >> java_thread->field_addr<jint>(_VTMS_transition_disable_count_offset); >> 1756: int val = AtomicAccess::load(addr); >> 1757: AtomicAccess::store(addr, val + 1); > > Suggestion: > > AtomicAccess::inc(addr); Same here. > src/hotspot/share/classfile/javaClasses.cpp line 1764: > >> 1762: jint* addr = >> java_thread->field_addr<jint>(_VTMS_transition_disable_count_offset); >> 1763: int val = AtomicAccess::load(addr); >> 1764: AtomicAccess::store(addr, val - 1); > > Suggestion: > > AtomicAccess::dec(addr); I’d prefer to leave it as a plain store to avoid the unnecessary extra fence. > src/hotspot/share/opto/runtime.hpp line 740: > >> 738: return vthread_transition_Type(); >> 739: } >> 740: > > I do not know C2 but this looks really strange - 4 different functions all > return the same thing. ??? We need to define them because the `GEN_C2_STUB` macro will look for the type of the C function based on its name (`C2_STUB_TYPEFUNC(name)`), otherwise we get a compilation failure. The four C functions have the same type though so they all return `_vthread_transition_Type`. > src/hotspot/share/runtime/handshake.cpp line 374: > >> 372: JavaThread* target = java_lang_Thread::thread(carrier_thread); >> 373: assert(target != nullptr, ""); >> 374: // Technically there is need for a ThreadsListHandle since the >> target > > Suggestion: > > // Technically there is no need for a ThreadsListHandle since the target > > ? Yes, fixed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561198741 PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561198549 PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561200538 PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561202212
