RFR: 8252842: Extend jmap to support parallel heap dump

[Lin Zang]

On Wed, 3 Feb 2021 22:02:04 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Hi Lin, When I suggested that it would ok just to just always use the default number of parallel threads, I wasn't considering that we also have `jmap -histo`, which also supports parallel threads, but includes an argument to specify it. For example, `jmap -histo:parallel=<n>`. Now this PR adds parallel support to `jmap -dump`, but you are suggesting not to have a parallel option to specify it, and use a default number of threads, which would be the same as what you would get with `jmap -histo:parallel=0`.  So this is inconsistent. If `-histo` is going to allow you to control the number of parallel threads, then `-dump` should also. I guess an option would be to not allow `-histo` to control the number of threads, and have it work just like you are proposing for `-dump`.
>
> Here's another thought. What if we added a new Attach API command called something like `setparallel`. Then when you specify `parallel=<n>` for  `jmap -histo` or `jmap -dump`, first JMap would send the `setparallel` command, and then it would send the `dumpheap` or `inspectheap` commands, and they would use the parallel setting from the previous `setparallel` command. I assume of `setparallel` is sent to an older JDK, I think you just get an error message back that can be ignored.
> 
> Anyway, just throwing this idea out there. I'm not so sure I like it myself, but given we don't have a good solution yet, it may be as good as any of the others.

Hi Chris,

> Hi Lin, When I suggested that it would ok just to just always use the default number of parallel threads, I wasn't considering that we also have `jmap -histo`, which also supports parallel threads, but includes an argument to specify it. For example, `jmap -histo:parallel=<n>`. Now this PR adds parallel support to `jmap -dump`, but you are suggesting not to have a parallel option to specify it, and use a default number of threads, which would be the same as what you would get with `jmap -histo:parallel=0`. So this is inconsistent. If `-histo` is going to allow you to control the number of parallel threads, then `-dump` should also. I guess an option would be to not allow `-histo` to control the number of threads, and have it work just like you are proposing for `-dump`.

I agree that -dump and -histo should keep consistent with "parallel" options. IMHO, removing "parallel" option for "-histo" from (maybe) JDK17 which was just introduced in JDK16 may confuse users. so maybe we could think of introducing "parallel" to "-dump". 


> Here's another thought. What if we added a new Attach API command called something like `setparallel`. Then when you specify `parallel=<n>` for `jmap -histo` or `jmap -dump`, first JMap would send the `setparallel` command, and then it would send the `dumpheap` or `inspectheap` commands, and they would use the parallel setting from the previous `setparallel` command. I assume of `setparallel` is sent to an older JDK, I think you just get an error message back that can be ignored.
> 
> Anyway, just throwing this idea out there. I'm not so sure I like it myself, but given we don't have a good solution yet, it may be as good as any of the others.

I think this is a good idea,  I will try it to see whether there is any problem.
IMO, there can be two ways which may help solve the compatibility issue of introducing "parallel" option:
- unblock socket with timeout. In this way, old Jmap could work with new JDK, but it maybe complicated because of different implementation for different OS. 
- Two times communication as you mentioned,  it sounds a reasonable solution, I will work out a prototype and then discuss with you . 

P.S.  I will re-open the CSR when we make the final decision on introducing "parallel" options. 
Thanks!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2261