RFR: JDK-8306441: Two phase segmented heap dump [v14]

[Yi Yang]

On Wed, 5 Jul 2023 21:10:56 GMT, Alex Menkov <amenkov at 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