On Wed, 13 Dec 2023 04:59:16 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>>> So is the main thread operating in_native whilst doing the merge? I suspect >>> the admonition of not doing the merge at a safepoint actually meant "not a >>> safepoint by the VMThread" as that would cause the whole VM to pause. Even >>> doing it in the VMThread at all can delay the next safepoint, which does >>> not seem good. >> >> I think the thread is in_vm. >> Main java thread call Runtime.gc(), GC itself calls heap dumper before/after >> full gc. >> I don't know if there is a performance difference in the case between >> executing merge on the current thread and VMThread. >> HeapDumpBeforeFullGC/HeapDumpAfterFullGC/HeapDumpOnOutOfMemoryError are >> diagnostic flags, so I think performance is not so important here. >> There are plans to improve their performance by turning on parallel dump >> (using GC worker) - often parallel heap dump + merge takes less time than >> single-threaded dump (we have now). >> >>> I'm not familiar with this code in general (and only looked at this because >>> of the previous issue in the CI) but I'm unclear what including virtual >>> thread stack referencves has to do with the merging logic? I would not >>> expect merging to be affected by the current change. >> >> This is caused by a nature of unmounted virtual threads. >> We need to dump stack traces before we dump heap objects, but we can find >> unmounted virtual threads only by iterating over heap. So we need a way to >> add data to the beginning of the dump file while dumping heap objects. >> Segmented dump helps here - we have a designated segment to write stack >> traces. > > I tested different scenarios (all I know) of heap dump: > - via attach (jcmd); > - JMX (HotSpotDiagnosticMXBean); > - HeapDumpBeforeFullGC/HeapDumpAfterFullGC; > - HeapDumpOnOutOfMemoryError. > I see no problem with merge on the current thread. > Only HeapDumpBeforeFullGC/HeapDumpAfterFullGC causes heap dump (and merge) at > safepoint. > But HeapDumpBeforeFullGC/HeapDumpAfterFullGC/HeapDumpOnOutOfMemoryError > depend on GC and require much more testing, I think it's better to integrate > this fix as it is and update the way merger is executed by a separate > follow-up change (with cleanup of related stuff) Okay. I'm concerned that the merge, whether by the current thread whilst in_vm, or by the VMThread, will either prevent the VM from going to a safepoint in a timely manner, or cause the safepoint to be excessively long. I'm not sure how problems in this area will become visible. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17040#discussion_r1424940134
