On Wed, 5 Jul 2023 21:10:56 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> I think commit "use HandshakeClosure instead of VMOperation" is a wrong way >> to go. >> It restricts use of parallel dumping only to attach case. >> I have pending changes in heap dumper to support virtual threads and I'm >> going switch heap dumper to always use DumpMerger. > >> Hi @alexmenkov, >> >> > It restricts use of parallel dumping only to attach case. >> >> I'm not sure what you mean. Using handshake won't limit it to only attach >> cases; it can also be used in other cases like HeapDumpBeforeFullGC. The >> only difference is that previously, VMThread merged the dump files, and now >> it's the Attach listener that merges them. For the file merging process, my >> candidate options are as follows: >> >> 1. Execute with VMThread outside the safepoint, which will block >> VMThread from executing other vmoperations. >> >> 2. Execute with Attach listener thread outside the safepoint, which will >> block Attach listener from processing requests like jcmd/jstack. >> >> 3. Create a new temporary thread to execute the file merging outside the >> safepoint, which will have some resource consumption. >> >> >> I don't have a strong motivation to use 2, and 3 may be a good solution with >> the availability of virtual threads. If you have any concerns, we can >> consider using the most conservative option 1 to simplify the review >> process, and then optimize the file merging process in a follow-up patch. > > I mean that you can't be sure that you can use attach listener thread. > My concerns about attach listener thread are: > - AttachListener can be disabled at all or fail to initialize; > - attach listener thread may be not yet available when we need to perform > heap dump; > - need to ensure attach listener thread can't be blocked (for example waiting > for next command) > > I think it makes sense to go option 1 now and add optimizations as follow-up > changes (they will be much smaller and easier to review). Thanks @alexmenkov for the reviews! I added corresponding jtreg for it, also I found verifyHeapDump is duplicated in several tests, I filed https://bugs.openjdk.org/browse/JDK-8311775 as a follow-up test improvement. @plummercj @kevinjwalls Can I have a second review when you have time? Thanks. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1628299494
