JDK-8261034: improve jcmd GC.class_histogram to support parallel #2379
Conversation
|
👋 Welcome back mli! A progress list of the required criteria for merging this PR into |
|
@Hamlin-Li The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
I think you'll need a CSR just as 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.
| _parallel_thread_num("-parallel", "parallel threads number for heap iteration", | ||
| "INT", false, "0") { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
/label add serviceability |
|
@plummercj |
|
Hi @Hamlin-Li |
That test is for the |
Correct! Thanks for point it out, and sorry for misleading the test case. |
Hi @linzang , it's ok, anyone can review. :-) |
|
@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. E: Could not configure 'libc6:i386'. |
|
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. |
|
/csr |
|
@Hamlin-Li this pull request will not be integrated until the CSR request JDK-8261105 for issue JDK-8261034 has been approved. |
|
Mailing list message from David Holmes on hotspot-runtime-dev: On 4/02/2021 5:39 pm, Hamlin Li wrote:
I believe this has been fixed, so your repo may be missing that David |
Hi David, Thank you for the information. |
| {"-parallel=0"}, | ||
| {"-parallel=1"}, | ||
| {"-parallel=2"}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sure, just added some test cases for invalid args, and also added test condition in production to return if parallel < 0.
| "parallel threads number for heap iteration. " | ||
| "0 use system determined number of threads, " | ||
| "1 use one thread, i.e., disable parallelism, " | ||
| "n use n threads, n must be positive.", |
There was a problem hiding this comment.
"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.",
There was a problem hiding this comment.
Thanks for detailed review, Chris, just modified as you suggested.
| "BOOLEAN", false, "false"), | ||
| _parallel_thread_num("-parallel", | ||
| "Number of parallel threads for heap iteration. " | ||
| "0 means let the VM determined the number of threads. " |
There was a problem hiding this comment.
Should be "determine". Sorry that was a typo in the text I suggested.
There was a problem hiding this comment.
My bad, thanks for pointing out. :-)
|
Hi @Hamlin-Li @plummercj, @sspitsyn Thanks, |
|
Thanks Lin for reminding. |
|
Seems the build failures on windows are not related to this change. |
|
@plummercj @sspitsyn Kindly reminder. Thanks. |
|
@Hamlin-Li This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 1646 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks @plummercj @sspitsyn for your review. /integrate |
openjdk
bot
added
integrated
|
@Hamlin-Li Since your change was applied there have been 1655 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 3c47cab. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
@Hamlin-Li The command |
10 similar comments
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
|
@Hamlin-Li The command |
parallel -histo of jmap was added by JDK-8214535, it's better to support parallel for jcmd GC.class_histogram too.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2379/head:pull/2379$ git checkout pull/2379Update a local copy of the PR:
$ git checkout pull/2379$ git pull https://git.openjdk.java.net/jdk pull/2379/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2379View PR using the GUI difftool:
$ git pr show -t 2379Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2379.diff