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

[Hamlin Li-2]

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

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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2379/files
  - new: https://git.openjdk.java.net/jdk/pull/2379/files/2b517feb..c083f6bd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2379&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2379&range=01-02

  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2379.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2379/head:pull/2379

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

[Chris Plummer-2]

On Thu, 4 Feb 2021 07:28:03 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.
>
> 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

test/hotspot/jtreg/serviceability/dcmd/gc/ClassHistogramTest.java line 96:

> 94:                 {"-parallel=0"},
> 95:                 {"-parallel=1"},
> 96:                 {"-parallel=2"},

Is there a way to test invalid arguments within this test framework? It seems the assumption for the `run()` method is that the arguments are valid and a histogram should be in the output.

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

> 560:        "0 use system determined number of threads, "
> 561:        "1 use one thread, i.e., disable parallelism, "
> 562:        "n use n threads, n must be positive.",

"Number of parallel threads for heap iteration. "
       "0 means let the VM determined the number of threads. "
       "1 means use one thread, i.e. disable parallelism. "
       "n means use n threads. n must be positive.",

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

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

[Hamlin Li-2]

On Thu, 4 Feb 2021 23:08:21 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
>
> test/hotspot/jtreg/serviceability/dcmd/gc/ClassHistogramTest.java line 96:
>
>> 94:                 {"-parallel=0"},
>> 95:                 {"-parallel=1"},
>> 96:                 {"-parallel=2"},
>
> Is there a way to test invalid arguments within this test framework? It seems the assumption for the `run()` method is that the arguments are valid and a histogram should be in the output.

sure, just added some test cases for invalid args, and also added test condition in production to return if parallel < 0.

> src/hotspot/share/services/diagnosticCommand.cpp line 562:
>
>> 560:        "0 use system determined number of threads, "
>> 561:        "1 use one thread, i.e., disable parallelism, "
>> 562:        "n use n threads, n must be positive.",
>
> "Number of parallel threads for heap iteration. "
>        "0 means let the VM determined the number of threads. "
>        "1 means use one thread, i.e. disable parallelism. "
>        "n means use n threads. n must be positive.",

Thanks for detailed review, Chris, just modified as you suggested.

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

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

[Serguei Spitsyn-2]

On Fri, 5 Feb 2021 02:40:13 GMT, Hamlin Li <[hidden email]> wrote:

>> src/hotspot/share/services/diagnosticCommand.cpp line 562:
>>
>>> 560:        "0 use system determined number of threads, "
>>> 561:        "1 use one thread, i.e., disable parallelism, "
>>> 562:        "n use n threads, n must be positive.",
>>
>> "Number of parallel threads for heap iteration. "
>>        "0 means let the VM determined the number of threads. "
>>        "1 means use one thread, i.e. disable parallelism. "
>>        "n means use n threads. n must be positive.",
>
> Thanks for detailed review, Chris, just modified as you suggested.

I like this approach - good idea.

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

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