RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads

[Alex Menkov]

On Thu, 21 Dec 2023 12:23:35 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> I mean race between virtual thread state change and the thread stack switch (to/from carrier).
>> Correct condition here is "dump the virtual thread  if it was not dumped as mounted virtual thread".
>> Most likely for RUNNABLE/PINNED/TIMED_PINNED we can be sure the thread is mounted, but we can get vthread in transition (PARKING, YIELDING, etc). Virtual threads may be in transition at safepoints (but they can't change mounted/unmounted state).
>> So I think `is_vthread_mounted` can be implemented in 2 ways:
>> 1) copy logic of JvmtiEnvBase::get_JavaThread_or_null:
>>   get JavaThread for java_lang_VirtualThread::carrier_thread(vt);
>>   if not null, check Continuation::is_continuation_mounted(java_thread, java_lang_VirtualThread::continuation(vt)) - this is to handle transition, when vthread is already unmounted, but carrierThread is not yet set to null;
>> 2) check that java_lang_VirtualThread::continuation(vt) doesn't have non-empty chunk.
>>   AFAIU this is true for mounted vthreads. If we get it for unmounted vt, its stack trace of the thread is empty anyway, so it's ok to skip it (most likely it can happen only if thread state is NEW or TERMINATED, we already skip such vthreads).
>> 
>> @AlanBateman could you please comment if my understanding is correct
>
>> I mean race between virtual thread state change and the thread stack switch (to/from carrier).
> 
> I'm not sure if I understand you correctly or if we can call it a race. Alan will correct me if I'm wrong.
> You are talking about thread state change. At least, mount state transition happens on the same JavaThread (it seems, you call it thread state switch). Mount state transition can go over a safepoint. But it should not progress while in a safepoint. David pointed out, "this was all happening at a global safepoint". My understanding is this assumption is correct. Then your approach `to identify that a virtual thread is mounted or not` should work in general. The condition `java_lang_VirtualThread::carrier_thread(vt) != nullptr` should indicate that the `vt` is mounted or is being in mount or unmount transition.
> If the `vt` is in mount or unmount transition then (it is a gray zone) the way we identify mounted state should match the way we did it when dumped mounted virtual threads.
> It is done this way: `oop mounted_vt = thread->is_vthread_mounted() ? thread->vthread() : nullptr;`
> So, it seems any of yous suggestion should work here. Though, it would be nice to simplify it a little if possible. Again, to be consistent, a `vt` in mount state transition just have to be identified as mounted or unmounted in both fragments in a similar way .

Looks like "race" is wrong word here. There is no race between different threads, we just cannot rely on vt state or carrierThread value when the thread in mount/unmount transition.
Sorry for the confusion.
Serguei, thank you for the analysis. I agree, the code for mounted and unmounted vthreads should be consistent.
For unmounted threads we have to get JavaThread of the carrier thread and if it's not null, check java_thread->is_vthread_mounted().
We don't need to check `is_continuation_mounted` as we are at safepoint.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1434583304