On Fri, 30 Jun 2023 22:23:19 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> Yi Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> memory leak > > 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). ------------- PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1622516883
