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

classic Classic list List threaded Threaded
41 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
On Wed, 3 Feb 2021 12:44:53 GMT, Hamlin Li <[hidden email]> wrote:

> parallel -histo of jmap was added by JDK-8214535, it's better to support parallel for jcmd GC.class_histogram too.

I think you'll need a CSR just as [JDK-8215624](https://bugs.openjdk.java.net/browse/JDK-8215624). Also, a test is needed. There are a couple of tests under `test/hotspot/jtreg/serviceability/dcmd/gc` that already test `GC.class_histogram`. You could probably clone or modify one of them to test the new `-parallel` option.

src/hotspot/share/services/diagnosticCommand.cpp line 559:

> 557:        "BOOLEAN", false, "false"),
> 558:   _parallel_thread_num("-parallel", "parallel threads number for heap iteration",
> 559:        "INT", false, "0")     {

Does "0" mean use a default number of parallel threads as it does for the jmap? It's unclear, but it seems that it doesn't, which I think leads to eventually hitting this assert in `update_active_workers(uint v)`:
    assert(v != 0, "Trying to set active workers to 0");
If that's not the case, please explain how 0 is handled. In any case, I think it should be made consistent with jmap and should also be documented in the above  help output:
  parallel=<count> generate histogram using this many parallel threads, default 0
                     0 use system determined number of threads
                     1 use one thread, i.e., disable parallelism
                     n use n threads, n must be positive

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Lin Zang-2
On Wed, 3 Feb 2021 12:44:53 GMT, Hamlin Li <[hidden email]> wrote:

> parallel -histo of jmap was added by JDK-8214535, it's better to support parallel for jcmd GC.class_histogram too.

Hi @Hamlin-Li
I am not reviewer, but maybe you could add test for "parallel" in BasicJMapTest.java.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
On Thu, 4 Feb 2021 02:58:52 GMT, Lin Zang <[hidden email]> wrote:

>  I am not reviewer, but maybe you could add test for "parallel" in BasicJMapTest.java.

That test is for the `jmap` command, not the `GC.class_histogram` jcmd. As I pointed out, there are tests in `test/hotpot/jtreg/serviceability/dcmd/gc` that already test `GC.class_histogram`. That would be a better starting point.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Lin Zang-2
On Thu, 4 Feb 2021 03:40:26 GMT, Chris Plummer <[hidden email]> wrote:

> > I am not reviewer, but maybe you could add test for "parallel" in BasicJMapTest.java.
>
> That test is for the `jmap` command, not the `GC.class_histogram` jcmd. As I pointed out, there are tests in `test/hotpot/jtreg/serviceability/dcmd/gc` that already test `GC.class_histogram`. That would be a better starting point.

Correct!  Thanks for point it out, and sorry for misleading the test case.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261034: improve jcmd GC.class_histogram to support parallel [v3]

Hamlin Li-2
In reply to this post by Chris Plummer-2
On Wed, 3 Feb 2021 21:28:40 GMT, Chris Plummer <[hidden email]> wrote:

>> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   JDK-8261034: improve jcmd GC.class_histogram to support parallel
>
> src/hotspot/share/services/diagnosticCommand.cpp line 559:
>
>> 557:        "BOOLEAN", false, "false"),
>> 558:   _parallel_thread_num("-parallel", "parallel threads number for heap iteration",
>> 559:        "INT", false, "0")     {
>
> Does "0" mean use a default number of parallel threads as it does for the jmap? It's unclear, but it seems that it doesn't, which I think leads to eventually hitting this assert in `update_active_workers(uint v)`:
>     assert(v != 0, "Trying to set active workers to 0");
> If that's not the case, please explain how 0 is handled. In any case, I think it should be made consistent with jmap and should also be documented in the above  help output:
>   parallel=<count> generate histogram using this many parallel threads, default 0
>                      0 use system determined number of threads
>                      1 use one thread, i.e., disable parallelism
>                      n use n threads, n must be positive

Thanks Chris for reviewing, I have updated the patch as you suggested, would you mind to have another look at it?
CSR is created at https://bugs.openjdk.java.net/browse/JDK-8261105

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Hamlin Li-2
In reply to this post by Chris Plummer-2
On Wed, 3 Feb 2021 12:44:53 GMT, Hamlin Li <[hidden email]> wrote:

> parallel -histo of jmap was added by JDK-8214535, it's better to support parallel for jcmd GC.class_histogram too.

> Hi @Hamlin-Li
> I am not reviewer, but maybe you could add test for "parallel" in BasicJMapTest.java.

Hi @linzang , it's ok, anyone can review. :-)

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
In reply to this post by Chris Plummer-2
On Thu, 4 Feb 2021 07:37:03 GMT, Hamlin Li <[hidden email]> wrote:

>>>  I am not reviewer, but maybe you could add test for "parallel" in BasicJMapTest.java.
>>
>> That test is for the `jmap` command, not the `GC.class_histogram` jcmd. As I pointed out, there are tests in `test/hotpot/jtreg/serviceability/dcmd/gc` that already test `GC.class_histogram`. That would be a better starting point.
>
> @plummercj Hi Chris, I have a question which might not be related to the patch, my presubmit tests always failed on linux platforms, but I don't think the failure is related to my patch. Would you mind to share some point? I totally have no idea what's going wrong.
> The error messages are as following:
>
> E: Could not configure 'libc6:i386'.
> E: Could not perform immediate configuration on 'libgcc-s1:i386'. Please see man 5 apt.conf under APT::Immediate-Configure for details. (2)
> Error: Process completed with exit code 100.

From what I've seen it's normal for the 32-bit builds to fail. I'm not sure why they are even being built.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Hamlin Li-2
On Thu, 4 Feb 2021 07:51:35 GMT, Chris Plummer <[hidden email]> wrote:

> From what I've seen it's normal for the 32-bit builds to fail. I'm not sure why they are even being built.

Thanks for the info, then I think it's safe for me to ignore the failures in linux x86.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Hamlin Li-2
In reply to this post by Chris Plummer-2
On Wed, 3 Feb 2021 12:44:53 GMT, Hamlin Li <[hidden email]> wrote:

> parallel -histo of jmap was added by JDK-8214535, it's better to support parallel for jcmd GC.class_histogram too.

> Hi @Hamlin-Li,
>
> It looks good in general.
> A question: What is going to happen if the number of parallel threads passed to the command is too big? Is there any limit, and what has to be the expected behavior in such a case? I'll look at the CSR.
> Thanks,
> Serguei

Hi Serguer,
Thanks for reviewing.
First, it got truncated as uint by ClassHistogramDCmd, then it got capped by by total_workers of gang, so finally it will not exceed total_workers.
Hope this answer your questions.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
On Tue, 9 Feb 2021 07:57:24 GMT, Hamlin Li <[hidden email]> wrote:

>> parallel -histo of jmap was added by JDK-8214535, it's better to support parallel for jcmd GC.class_histogram too.
>
>> Hi @Hamlin-Li,
>>
>> It looks good in general.
>> A question: What is going to happen if the number of parallel threads passed to the command is too big? Is there any limit, and what has to be the expected behavior in such a case? I'll look at the CSR.
>> Thanks,
>> Serguei
>
> Hi Serguer,
> Thanks for reviewing.
> First, it got truncated as uint by ClassHistogramDCmd, then it got capped in HeapInspection by by total_workers of gang, so finally it will not exceed total_workers.
> Hope this answer your questions.

I think part of the concern here is that this is all hidden from the user. "total_workers of gang" is not something that is spec'd somewhere. So when the user chooses to set "parallel" to something other than 0 (the default), then there really is no guidance as to what a good alternative number is. Presumably you are setting it because you feel the default is too low or too high, but without knowing the actual number of threads used by default, it's hard to come up with a reasonable adjustment to specify. We also have the relationship to `-XX:ParallelGCThreads`, which impacts this.

Maybe the option should just be to enable or disable parallel. Before your changes it looks like you got parallel=1, so no parallel threads, and no ability to change that. With your changes the default is now parallel=0, so you get the default number of threads, and you can change it to any number. So maybe we should choose one of the following two simpler approaches:

- 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.

However, this probably needs to be consistent with the jmap command and the attach API protocol, which is being discussed in  #2261. They should both either choose one of the above simplified approaches, or do a better job of specifying defaults and limits for the number of threads. So some input from @linzang woudl be useful here.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Lin Zang-2
On Tue, 9 Feb 2021 19:13:55 GMT, Chris Plummer <[hidden email]> wrote:

>>> Hi @Hamlin-Li,
>>>
>>> It looks good in general.
>>> A question: What is going to happen if the number of parallel threads passed to the command is too big? Is there any limit, and what has to be the expected behavior in such a case? I'll look at the CSR.
>>> Thanks,
>>> Serguei
>>
>> Hi Serguer,
>> Thanks for reviewing.
>> First, it got truncated as uint by ClassHistogramDCmd, then it got capped in HeapInspection by by total_workers of gang, so finally it will not exceed total_workers.
>> Hope this answer your questions.
>
> I think part of the concern here is that this is all hidden from the user. "total_workers of gang" is not something that is spec'd somewhere. So when the user chooses to set "parallel" to something other than 0 (the default), then there really is no guidance as to what a good alternative number is. Presumably you are setting it because you feel the default is too low or too high, but without knowing the actual number of threads used by default, it's hard to come up with a reasonable adjustment to specify. We also have the relationship to `-XX:ParallelGCThreads`, which impacts this.
>
> Maybe the option should just be to enable or disable parallel. Before your changes it looks like you got parallel=1, so no parallel threads, and no ability to change that. With your changes the default is now parallel=0, so you get the default number of threads, and you can change it to any number. So maybe we should choose one of the following two simpler approaches:
>
> - 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.
>
> However, this probably needs to be consistent with the jmap command and the attach API protocol, which is being discussed in  #2261. They should both either choose one of the above simplified approaches, or do a better job of specifying defaults and limits for the number of threads. So some input from @linzang woudl be useful here.

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

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Hamlin Li-2
On Wed, 10 Feb 2021 00:34:02 GMT, Lin Zang <[hidden email]> wrote:

> > * 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.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Lin Zang-2
On Wed, 10 Feb 2021 01:49:02 GMT, Hamlin Li <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Hamlin Li-2
On Wed, 10 Feb 2021 02:21:03 GMT, Lin Zang <[hidden email]> wrote:

>>> > * 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

Hi Lin,
Sure, let's discuss -histo in this thread first.

Hi @sspitsyn @plummercj @linzang
IMHO, maybe we could make the option simple and automatically speed up heap scan by disabling "parallel=" option and adding "noparallel" at the same time? From user's point of view, "noparallel" will speed up their histo/dump automatically, if they are migrating from jdk15 or prior to jdk15, and also give them a way to fall back to previous serial scan if they would like to do that.
Although This way will introduce the compability issue, fortunately JDK16 is not LTS, and maybe we could backport to 16 updates if necessary?
How do you think about it? Thanks for discussion.

( BTW, I might not be able to response timely, as it will be long public holiday the next few days, I'm sorry for the inconvenience. )

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
On Wed, 10 Feb 2021 08:13:33 GMT, Hamlin Li <[hidden email]> wrote:

>> 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
>
> Hi Lin,
> Sure, let's discuss -histo in this thread first.
>
> Hi @sspitsyn @plummercj @linzang
> My following point is based on my humble opinion that parallel=N is not necessary for user and we should use the largest parallel capability of JVM to do heap histo when user intend to do it in parallel. But if you think this basic opinion is not that correct, please kindly ignore following opinion. :-)
> IMHO, maybe we could make the option simple and automatically speed up heap scan by disabling "parallel=" option and adding "noparallel" at the same time? From user's point of view, "noparallel" will speed up their histo/dump automatically, if they are migrating from jdk15 or prior to jdk15, and also give them a way to fall back to previous serial scan if they would like to do that. Although This way will introduce the compability issue, fortunately JDK16 is not LTS, and maybe we could backport to 16 updates if necessary?
> How do you think about it? Thanks for discussion.
>
> ( BTW, I might not be able to response timely, as it will be long public holiday the next few days, I'm sorry for the inconvenience. )

I'm in favor of just having a `noparallel` option (default to using parallel) instead of using `parallel=<n>`. This would be for both the histo and heap dump features, and for both the `jmap` and `jcmd` support of these features.

As for the current `parallel=<n>` support for jmap histo already in JDK 16, hopefully there would be no objections to removing that in JDK 17 in favor of the `noparallel` approach, but I think the CSR for that change should get some good scrutiny. I think it would good to get approval from @AlanBateman and/or @dholmes-ora since they are both in the CSR group and have good serviceability knowledge.

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Lin Zang-2
On Wed, 10 Feb 2021 22:47:28 GMT, Chris Plummer <[hidden email]> wrote:

>> Hi Lin,
>> Sure, let's discuss -histo in this thread first.
>>
>> Hi @sspitsyn @plummercj @linzang
>> My following point is based on my humble opinion that parallel=N is not necessary for user and we should use the largest parallel capability of JVM to do heap histo when user intend to do it in parallel. But if you think this basic opinion is not that correct, please kindly ignore following opinion. :-)
>> IMHO, maybe we could make the option simple and automatically speed up heap scan by disabling "parallel=" option and adding "noparallel" at the same time? From user's point of view, "noparallel" will speed up their histo/dump automatically, if they are migrating from jdk15 or prior to jdk15, and also give them a way to fall back to previous serial scan if they would like to do that. Although This way will introduce the compability issue, fortunately JDK16 is not LTS, and maybe we could backport to 16 updates if necessary?
>> How do you think about it? Thanks for discussion.
>>
>> ( BTW, I might not be able to response timely, as it will be long public holiday the next few days, I'm sorry for the inconvenience. )
>
> I'm in favor of just having a `noparallel` option (default to using parallel) instead of using `parallel=<n>`. This would be for both the histo and heap dump features, and for both the `jmap` and `jcmd` support of these features.
>
> As for the current `parallel=<n>` support for jmap histo already in JDK 16, hopefully there would be no objections to removing that in JDK 17 in favor of the `noparallel` approach, but I think the CSR for that change should get some good scrutiny. I think it would good to get approval from @AlanBateman and/or @dholmes-ora since they are both in the CSR group and have good serviceability knowledge.
>
> For reference, the JDK 16 CSR for adding `parallel=<n>` support to `jmap -histo` is [8239290](https://bugs.openjdk.java.net/browse/JDK-8239290), and the bug to change this to a `noparallel` opiton in JDK 17 is [JDK-8261482](https://bugs.openjdk.java.net/browse/JDK-8261482). I don't think there is a CSR yet.

Dear All,
I took the liberity to create a PR (#2519) for the "noparallel" option. Maybe we could discuss there.  And I also found @Hamlin-Li has created a CSR for that. Thanks a lot!

BRs,
Lin

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
On Thu, 11 Feb 2021 02:39:55 GMT, Lin Zang <[hidden email]> wrote:

>> I'm in favor of just having a `noparallel` option (default to using parallel) instead of using `parallel=<n>`. This would be for both the histo and heap dump features, and for both the `jmap` and `jcmd` support of these features.
>>
>> As for the current `parallel=<n>` support for jmap histo already in JDK 16, hopefully there would be no objections to removing that in JDK 17 in favor of the `noparallel` approach, but I think the CSR for that change should get some good scrutiny. I think it would good to get approval from @AlanBateman and/or @dholmes-ora since they are both in the CSR group and have good serviceability knowledge.
>>
>> For reference, the JDK 16 CSR for adding `parallel=<n>` support to `jmap -histo` is [8239290](https://bugs.openjdk.java.net/browse/JDK-8239290), and the bug to change this to a `noparallel` opiton in JDK 17 is [JDK-8261482](https://bugs.openjdk.java.net/browse/JDK-8261482). The CSR is [JDK-8261543](https://bugs.openjdk.java.net/browse/JDK-8261543).
>
> Dear All,
> I took the liberity to create a PR (#2519) for the "noparallel" option. Maybe we could discuss there.  And I also found @Hamlin-Li has created a CSR for that. Thanks a lot!
>
> BRs,
> Lin

There is still one minor issue with this `noparallel` support, and that is we still run into the problem of `jmap -dump` not being able to pass a 4th argument via the attach API. We could just say that's fine, and require using the `jcmd` to do the heap dump if the user wants to override the default of having parallel enabled, but then it seems we should do the same for `jmap -histo` to be consistent. So in other words, no support in `jmap` for disabling parallel support, but we do have the support in `jcmd`. Thoughts?

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Lin Zang-2
On Thu, 11 Feb 2021 22:50:50 GMT, Chris Plummer <[hidden email]> wrote:

>> Dear All,
>> I took the liberity to create a PR (#2519) for the "noparallel" option. Maybe we could discuss there.  And I also found @Hamlin-Li has created a CSR for that. Thanks a lot!
>>
>> BRs,
>> Lin
>
> There is still one minor issue with this `noparallel` support, and that is we still run into the problem of `jmap -dump` not being able to pass a 4th argument via the attach API. We could just say that's fine, and require using the `jcmd` to do the heap dump if the user wants to override the default of having parallel enabled, but then it seems we should do the same for `jmap -histo` to be consistent. So in other words, no support in `jmap` for disabling parallel support, but we do have the support in `jcmd`. Thoughts?

Hi Chris,

> There is still one minor issue with this `noparallel` support, and that is we still run into the problem of `jmap -dump` not being able to pass a 4th argument via the attach API. We could just say that's fine, and require using the `jcmd` to do the heap dump if the user wants to override the default of having parallel enabled, but then it seems we should do the same for `jmap -histo` to be consistent. So in other words, no support in `jmap` for disabling parallel support, but we do have the support in `jcmd`. Thoughts?

I just updated the #2261 which introduce a new command "dumpheapext" that could handle more arguments (as you suggested the idea :-> ). I think maybe that kind of change is acceptable. Moreover, this way also allow us to add more options in future.

So maybe we have two choice here:
- Add a new command for argument extension.   so jcmd and jmap could be consisitent.
- Remove the "parallel" option in jmap, and leave the control to jcmd.

I prefer the first one as it may be more clearer for users to have consisitent usage of jcmd and jmap tools as they were before.

Thanks!
Lin

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
On Fri, 12 Feb 2021 01:03:28 GMT, Lin Zang <[hidden email]> wrote:

> So maybe we have two choice here:
>     * Add a new command for argument extension.   so jcmd and jmap could be consisitent.
>     * Remove the "parallel" option in jmap, and leave the control to jcmd.

Yes, I suppose we can still do the `dumpheapext` approach to allow for having 4 args. It's a bit of a hack, but at least it is hidden from the user, and allows `jmap` and `jcmd` to both support `noparallel`

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

PR: https://git.openjdk.java.net/jdk/pull/2379
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer-2
On Fri, 12 Feb 2021 06:25:41 GMT, Chris Plummer <[hidden email]> wrote:

>> Hi Chris,
>>
>>> There is still one minor issue with this `noparallel` support, and that is we still run into the problem of `jmap -dump` not being able to pass a 4th argument via the attach API. We could just say that's fine, and require using the `jcmd` to do the heap dump if the user wants to override the default of having parallel enabled, but then it seems we should do the same for `jmap -histo` to be consistent. So in other words, no support in `jmap` for disabling parallel support, but we do have the support in `jcmd`. Thoughts?
>>
>> I just updated the #2261 which introduce a new command "dumpheapext" that could handle more arguments (as you suggested the idea :-> ). I think maybe that kind of change is acceptable. Moreover, this way also allow us to add more options in future.
>>
>> So maybe we have two choice here:
>> - Add a new command for argument extension.   so jcmd and jmap could be consisitent.
>> - Remove the "parallel" option in jmap, and leave the control to jcmd.
>>
>> I prefer the first one as it may be more clearer for users to have consisitent usage of jcmd and jmap tools as they were before.
>>
>> Thanks!
>> Lin
>
>> So maybe we have two choice here:
>>     * Add a new command for argument extension.   so jcmd and jmap could be consisitent.
>>     * Remove the "parallel" option in jmap, and leave the control to jcmd.
>
> Yes, I suppose we can still do the `dumpheapext` approach to allow for having 4 args. It's a bit of a hack, but at least it is hidden from the user, and allows `jmap` and `jcmd` to both support `noparallel`

Sigh, I've keep debating this with myself, and keep having second thoughts on what is best. I'm now leaning towards just sticking with `parallel=<n>`, and for any n > 1, at most we just say the user will need to experiment to see what works best (if we even say that). This way we avoid any headaches with the the JDK 16 support for `parallel=<n>` with `jmap -histo`. I know this isn't ideal, but I'm guessing for the most part users will just leave this option alone anyway, and will get the default number of threads. But, my current feelings here also hinge on adding `dumpheapext` so `-dump` and `-histo` both support parallel options.

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

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