On Wed, 3 Feb 2021 22:02:04 GMT, Chris Plummer <cjplum...@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
