Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads [v4]
On Fri, 22 Dec 2023 20:52:48 GMT, Alex Menkov wrote: >> HeapDumper dumps virtual threads in 2 places: >> - dumping platform threads (mounted virtual threads are dumped as separate >> thread object); >> - dumping heap objects when the object is `java.lang.VirtualThread`. >> >> In the 2nd case mounted virtual threads should be skipped (as they are >> already dumped with correct stack traces/stack references) >> Check that a virtual thread is mounted is non-trivial, method from >> JvmtiEnvBase was used for this. >> >> Testing: tier1..3, heapdump-related tests: >> open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > feedback: extended comment Thank you for the update! - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17134#pullrequestreview-179865
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads [v4]
> HeapDumper dumps virtual threads in 2 places: > - dumping platform threads (mounted virtual threads are dumped as separate > thread object); > - dumping heap objects when the object is `java.lang.VirtualThread`. > > In the 2nd case mounted virtual threads should be skipped (as they are > already dumped with correct stack traces/stack references) > Check that a virtual thread is mounted is non-trivial, method from > JvmtiEnvBase was used for this. > > Testing: tier1..3, heapdump-related tests: > open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback: extended comment - Changes: - all: https://git.openjdk.org/jdk/pull/17134/files - new: https://git.openjdk.org/jdk/pull/17134/files/c6ad90ca..011ed399 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17134&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17134&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17134.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17134/head:pull/17134 PR: https://git.openjdk.org/jdk/pull/17134
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads [v3]
On Fri, 22 Dec 2023 01:43:07 GMT, Alex Menkov wrote: >> HeapDumper dumps virtual threads in 2 places: >> - dumping platform threads (mounted virtual threads are dumped as separate >> thread object); >> - dumping heap objects when the object is `java.lang.VirtualThread`. >> >> In the 2nd case mounted virtual threads should be skipped (as they are >> already dumped with correct stack traces/stack references) >> Check that a virtual thread is mounted is non-trivial, method from >> JvmtiEnvBase was used for this. >> >> Testing: tier1..3, heapdump-related tests: >> open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > feedback: comment added The fix looks good. Thank you for the update. Also, added a minor comment. src/hotspot/share/services/heapDumper.cpp line 1934: > 1932: // create a HPROF_GC_INSTANCE record for each object > 1933: DumperSupport::dump_instance(writer(), o, &_class_cache); > 1934: // If we encounter an unmounted virtual thread it needs to be > dumped explicitly. Nit: It is nice to have a comment here. I'm thinking if it'd make sens to shortly explain why mounted threads are not dumped here. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17134#pullrequestreview-1794318747 PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1434952089
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads [v2]
On Fri, 22 Dec 2023 00:44:16 GMT, David Holmes wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> feedback: reimplemented ThreadDumpe::is_vthread_mounted() > > src/hotspot/share/services/heapDumper.cpp line 1934: > >> 1932: // create a HPROF_GC_INSTANCE record for each object >> 1933: DumperSupport::dump_instance(writer(), o, &_class_cache); >> 1934: if (java_lang_VirtualThread::is_instance(o) > > Suggestion. Just to be clear add a comment > > // If we encounter an unmounted virtual thread it needs to be dumped > explicitly. > > or something to that effect. Thanks. Thank you for review Added comment as suggested - PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1434638886
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads [v3]
> HeapDumper dumps virtual threads in 2 places: > - dumping platform threads (mounted virtual threads are dumped as separate > thread object); > - dumping heap objects when the object is `java.lang.VirtualThread`. > > In the 2nd case mounted virtual threads should be skipped (as they are > already dumped with correct stack traces/stack references) > Check that a virtual thread is mounted is non-trivial, method from > JvmtiEnvBase was used for this. > > Testing: tier1..3, heapdump-related tests: > open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback: comment added - Changes: - all: https://git.openjdk.org/jdk/pull/17134/files - new: https://git.openjdk.org/jdk/pull/17134/files/0afd2a4f..c6ad90ca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17134&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17134&range=01-02 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17134.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17134/head:pull/17134 PR: https://git.openjdk.org/jdk/pull/17134
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads [v2]
On Fri, 22 Dec 2023 00:31:51 GMT, Alex Menkov wrote: >> HeapDumper dumps virtual threads in 2 places: >> - dumping platform threads (mounted virtual threads are dumped as separate >> thread object); >> - dumping heap objects when the object is `java.lang.VirtualThread`. >> >> In the 2nd case mounted virtual threads should be skipped (as they are >> already dumped with correct stack traces/stack references) >> Check that a virtual thread is mounted is non-trivial, method from >> JvmtiEnvBase was used for this. >> >> Testing: tier1..3, heapdump-related tests: >> open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > feedback: reimplemented ThreadDumpe::is_vthread_mounted() This seems good to me know - thanks for the updates. One minor suggestion below. src/hotspot/share/services/heapDumper.cpp line 1934: > 1932: // create a HPROF_GC_INSTANCE record for each object > 1933: DumperSupport::dump_instance(writer(), o, &_class_cache); > 1934: if (java_lang_VirtualThread::is_instance(o) Suggestion. Just to be clear add a comment // If we encounter an unmounted virtual thread it needs to be dumped explicitly. or something to that effect. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17134#pullrequestreview-1793820556 PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1434616383
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads [v2]
> HeapDumper dumps virtual threads in 2 places: > - dumping platform threads (mounted virtual threads are dumped as separate > thread object); > - dumping heap objects when the object is `java.lang.VirtualThread`. > > In the 2nd case mounted virtual threads should be skipped (as they are > already dumped with correct stack traces/stack references) > Check that a virtual thread is mounted is non-trivial, method from > JvmtiEnvBase was used for this. > > Testing: tier1..3, heapdump-related tests: > open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback: reimplemented ThreadDumpe::is_vthread_mounted() - Changes: - all: https://git.openjdk.org/jdk/pull/17134/files - new: https://git.openjdk.org/jdk/pull/17134/files/3fc64e3a..0afd2a4f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17134&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17134&range=00-01 Stats: 11 lines in 1 file changed: 9 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17134.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17134/head:pull/17134 PR: https://git.openjdk.org/jdk/pull/17134
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads
On Thu, 21 Dec 2023 12:23:35 GMT, Serguei Spitsyn 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
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads
On Tue, 19 Dec 2023 21:34:10 GMT, Alex Menkov wrote: > 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 . - PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1434006417
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads
On Tue, 19 Dec 2023 05:16:38 GMT, David Holmes wrote: >> Good point. I'll remove dependency on JVMTI. >> I don't think approximation would be good here (comparing state to >> RUNNABLE/PINNED/TIMED_PINNED or comparing carrierThread with null). >> It's racy and we have a chance to not dump unmounted vthread or dump mounted >> vthread twice. >> Maybe `is_vthread_mounted` should check if the virtual thread continuation >> has non-empty chunk. > > If that is racy then any solution is going to be racy. I assumed this was all > happening at a global safepoint, otherwise threads could be > mounting/unmounting at any time. > > I said "approximation" only because I'm unsure exactly when the thread state > gets updated in the mounting/unmounting process. 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 - PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1431959463
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads
On Tue, 19 Dec 2023 04:09:47 GMT, Alex Menkov wrote:
>> src/hotspot/share/services/heapDumper.cpp line 1647:
>>
>>> 1645: static bool is_vthread_mounted(oop vt) {
>>> 1646: return JvmtiEnvBase::get_JavaThread_or_null(vt) != nullptr;
>>> 1647: }
>>
>> It doesn't seem appropriate to couple this to the JVMTI code (can this code
>> be present if JVMTI is not part of the build?). Doesn't the VT state give
>> you a good enough approximation of whether it is mounted i.e. RUNNABLE?
>
> Good point. I'll remove dependency on JVMTI.
> I don't think approximation would be good here (comparing state to
> RUNNABLE/PINNED/TIMED_PINNED or comparing carrierThread with null).
> It's racy and we have a chance to not dump unmounted vthread or dump mounted
> vthread twice.
> Maybe `is_vthread_mounted` should check if the virtual thread continuation
> has non-empty chunk.
If that is racy then any solution is going to be racy. I assumed this was all
happening at a global safepoint, otherwise threads could be mounting/unmounting
at any time.
I said "approximation" only because I'm unsure exactly when the thread state
gets updated in the mounting/unmounting process.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1430924428
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads
On Tue, 19 Dec 2023 01:17:10 GMT, David Holmes wrote:
>> HeapDumper dumps virtual threads in 2 places:
>> - dumping platform threads (mounted virtual threads are dumped as separate
>> thread object);
>> - dumping heap objects when the object is `java.lang.VirtualThread`.
>>
>> In the 2nd case mounted virtual threads should be skipped (as they are
>> already dumped with correct stack traces/stack references)
>> Check that a virtual thread is mounted is non-trivial, method from
>> JvmtiEnvBase was used for this.
>>
>> Testing: tier1..3, heapdump-related tests:
>> open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb
>
> src/hotspot/share/services/heapDumper.cpp line 1647:
>
>> 1645: static bool is_vthread_mounted(oop vt) {
>> 1646: return JvmtiEnvBase::get_JavaThread_or_null(vt) != nullptr;
>> 1647: }
>
> It doesn't seem appropriate to couple this to the JVMTI code (can this code
> be present if JVMTI is not part of the build?). Doesn't the VT state give you
> a good enough approximation of whether it is mounted i.e. RUNNABLE?
Good point. I'll remove dependency on JVMTI.
I don't think approximation would be good here (comparing state to
RUNNABLE/PINNED/TIMED_PINNED or comparing carrierThread with null).
It's racy and we have a chance to not dump unmounted vthread or dump mounted
vthread twice.
Maybe `is_vthread_mounted` should check if the virtual thread continuation has
non-empty chunk.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1430893424
Re: RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads
On Sat, 16 Dec 2023 02:15:16 GMT, Alex Menkov wrote:
> HeapDumper dumps virtual threads in 2 places:
> - dumping platform threads (mounted virtual threads are dumped as separate
> thread object);
> - dumping heap objects when the object is `java.lang.VirtualThread`.
>
> In the 2nd case mounted virtual threads should be skipped (as they are
> already dumped with correct stack traces/stack references)
> Check that a virtual thread is mounted is non-trivial, method from
> JvmtiEnvBase was used for this.
>
> Testing: tier1..3, heapdump-related tests:
> open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb
src/hotspot/share/services/heapDumper.cpp line 1647:
> 1645: static bool is_vthread_mounted(oop vt) {
> 1646: return JvmtiEnvBase::get_JavaThread_or_null(vt) != nullptr;
> 1647: }
It doesn't seem appropriate to couple this to the JVMTI code (can this code be
present if JVMTI is not part of the build?). Doesn't the VT state give you a
good enough approximation of whether it is mounted i.e. RUNNABLE?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17134#discussion_r1430801839
RFR: JDK-8322237: Heap dump contains duplicate thread records for mounted virtual threads
HeapDumper dumps virtual threads in 2 places: - dumping platform threads (mounted virtual threads are dumped as separate thread object); - dumping heap objects when the object is `java.lang.VirtualThread`. In the 2nd case mounted virtual threads should be skipped (as they are already dumped with correct stack traces/stack references) Check that a virtual thread is mounted is non-trivial, method from JvmtiEnvBase was used for this. Testing: tier1..3, heapdump-related tests: open/test/hotspot/jtreg/serviceability,open/test/hotspot/jtreg/runtime/ErrorHandling,open/test/hotspot/jtreg/gc/epsilon,open/test/jdk/sun/tools/jhsdb - Commit messages: - jcheck - JDK-8322237 Changes: https://git.openjdk.org/jdk/pull/17134/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17134&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322237 Stats: 25 lines in 2 files changed: 23 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17134.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17134/head:pull/17134 PR: https://git.openjdk.org/jdk/pull/17134
