On Tue, 29 Oct 2024 02:56:30 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix comment in VThreadWaitReenter
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1082:
>
>> 1080: } else {
>> 1081: assert(vthread != nullptr, "no vthread oop");
>> 1082: oop oopCont = java_lang_VirtualThread::continuation(vthread);
>
> Nit: The name `oopCont` does not match the HotSpot naming convention.
> What about `cont_oop` or even better just `cont` as at the line 2550?
Renamed to cont.
> src/hotspot/share/prims/jvmtiExport.cpp line 1682:
>
>> 1680:
>> 1681: // On preemption JVMTI state rebinding has already happened so get
>> it always directly from the oop.
>> 1682: JvmtiThreadState *state =
>> java_lang_Thread::jvmti_thread_state(JNIHandles::resolve(vthread));
>
> I'm not sure this change is right. The `get_jvmti_thread_state()` has a role
> to lazily create a `JvmtiThreadState` if it was not created before. With this
> change the `JvmtiThreadState` creation can be missed if the `unmount` event
> is the first event encountered for this particular virtual thread. You
> probably remember that lazy creation of the `JvmtiThreadState`'s is an
> important optimization to avoid big performance overhead when a JVMTI agent
> is present.
Right, good find. I missed `get_jvmti_thread_state ` will also create the state
if null. How about this fix:
https://github.com/pchilano/jdk/commit/baf30d92f79cc084824b207a199672f5b7f9be88
I now also see that JvmtiVirtualThreadEventMark tries to save some state of the
JvmtiThreadState for the current thread before the callback, which is not the
JvmtiThreadState of the vthread for this case. Don't know if something needs to
change there too.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823319745
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823322449
