RFR: JDK-8261034: improve jcmd GC.class_histogram to support parallel

[Lin Zang]

On Wed, 10 Feb 2021 01:49:02 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi Chris, 
>> 
>>> * Default to not parallel. Allow the user to specify `-parallel`, but not specify the thread count. They will get the default number of threads just like `parallel=0`
>>> * Default to parallel using the default number of threads. Allow the user to turn parallel off with a `-noparallel` option.
>> 
>> I would prefer the -noparallel option that enable parallel by default. IMHO, the parallel heap inspection is a little bit like parallelGCThreads -  which allowing the hotspot to decide the parallel thread number based on the platfrom by default. 
>> 
>> And I remember Serguei (@sspitsyn) and I discussed the "-parallel" option at https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-September/032836.html,  we finally decide to not introduce it. just FYI. 
>> 
>>> However, this probably needs to be consistent with the jmap command and the attach API protocol, which is being discussed in #2261.
>> 
>> if just introducing new option like "parallel" or "noparallel", I think we don't need to bother about attach API protocol. we can preprocess it in JMap.java and then pass the value of "0" or "1" as parallel_thread_number, just like what it is implemented at present, no compatibility issue introduced :-)
>>  
>> 
>> Moreover, I am thinking maybe we could print a meesage like "inspected heap parallelly with `parallel_numer` threads" in the result,  just like -dump's output "Heap dump file created [18904360 bytes in 0.039 secs]"
>> 
>> Thanks, 
>> Lin
>
>> > * Default to not parallel. Allow the user to specify `-parallel`, but not specify the thread count. They will get the default number of threads just like `parallel=0`
>> > * Default to parallel using the default number of threads. Allow the user to turn parallel off with a `-noparallel` option.
>> 
>> I would prefer the -noparallel option that enable parallel by default. IMHO, the parallel heap inspection is a little bit like parallelGCThreads - which allowing the hotspot to decide the parallel thread number based on the platfrom by default.
>> 
> 
> Hi Chris, Lin,
> Thanks for discussion.
> I'm OK with either options, both ways simplify the spec and hide details from users.
> 
>> And I remember Serguei (@sspitsyn) and I discussed the "-parallel" option at https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-September/032836.html, we finally decide to not introduce it. just FYI.
>> 
>> > However, this probably needs to be consistent with the jmap command and the attach API protocol, which is being discussed in #2261.
>> 
>> if just introducing new option like "parallel" or "noparallel", I think we don't need to bother about attach API protocol. we can preprocess it in JMap.java and then pass the value of "0" or "1" as parallel_thread_number, just like what it is implemented at present, no compatibility issue introduced :-)
> 
> If we decide to take either of above ways, then we should modify the uint to bool, it's more clear.
> 
>> 
>> Moreover, I am thinking maybe we could print a meesage like "inspected heap parallelly with `parallel_numer` threads" in the result, just like -dump's output "Heap dump file created [18904360 bytes in 0.039 secs]"
> 
> Agree, it helps.
> 
> BTW, I think we have mixed several threads(jcmd, jmap histo/dump) together. How about we address jmap -histo first (I filed a new bug at https://bugs.openjdk.java.net/browse/JDK-8261482), then I adjust the behavior of jcmd accordingly?
> 
> Thanks.

Hi Hamlin,

> If we decide to take either of above ways, then we should modify the uint to bool, it's more clear.

Sorry that I am a little confuse about it:
- Does it mean to disable "parallel=" option to not let user tuning the parallel thread number?  
- Or just add a "parallel" or "noparallel" option to change the default behavior? 

Both looks fine with me. But IMHO, regardless the ablility of "tunning", disabling the "parallel=" option maybe cause more backward ability issue. User of JDK16 may get error when using option like "parallel=1".  Although I am not sure how this is important since the parallel options was just introduced to JDK16 and it is not LTS.

BTW, I am not sure whether adding "parallel" requre a new CSR as there already have "parallel=" option. And seems like adding "noparallel" require one, right?  
 
> BTW, I think we have mixed several threads(jcmd, jmap histo/dump) together. How about we address jmap -histo first (I filed a new bug at https://bugs.openjdk.java.net/browse/JDK-8261482), then I adjust the behavior of jcmd accordingly?

I agree, we could start with -histo first, maybe we can discuss in this thread first and then handle it in new PR? 

Thanks, 
Lin

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

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