New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8252842: Extend jmap to support parallel heap dump #2261
Conversation
👋 Welcome back lzang! A progress list of the required criteria for merging this PR into |
@linzang The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
/csr needed |
@linzang this pull request will not be integrated until the CSR request JDK-8260424 for issue JDK-8252842 has been approved. |
/label remove core-libs |
@AlanBateman |
// Not that making all arguments as a whole string like jcmd did does not guarantee the | ||
// compatiabilty when the new jmap is uses on old version of JDK. | ||
// See AttachOperation::arg_count_max in attachListener.hpp for argument count limitation. | ||
String more_args = compress_level + "," + parallel; | ||
// dumpHeap is not the same as jcmd GC.heap_dump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To-discuss:
Compatibility issue that new version of jmap work on old JDK may wrongly processing the following case:
jmap -dump:file=dump.hprof,gz=1,parallel=2
this will cause the gz option not recognized correctly.
Can not simply change arg_count_max to 4, because it could cause JDK hang on waiting for arguments when old jmap is used.
Dear All, But I want to firstly ask your help to discuss a potential compatibility issue that may need to solve: This patch introduces a new "parallel" option to jmap, so after this patch, there can be at most 4 options for jmap that can be passed to JDK. The issue is that there is a limitation in old version of jmap that at most 3 arguments can be accepted. As I commented in File JMap.java. and I can not simply enlarge the limitation to 4 because that will cause old version of jmap hang when working on new JDK. So I use the 3rd argument of jmap to be a combined string and parse it to JDK, and the attachListener.cpp is modified to add the logic for parsing it to get gzlevel and parallel thread number. The current status is that, with current change in this patch, old jmap could work normally on new JDK, and new jmap without "parallel" could work correctly with old JDK. But the problem comes when new jmap use "gz=1, parallel=2" options together to communicate to an old JDK, the "gz" option takes no effect. The root cause is that old JDK's attachListener.cpp does not have the parsing logic, so it treat "gz=1, parallel=2" as a whole argument, and hence can not recognize it. May I ask your suggestion to see whether this is behavior is acceptable, and if not, any advice for fixing the compatibility issue? Thanks! |
A year or so ago we ran into this same issue. I'll see if I can find the CR for it. |
It looks like JDK-8215622 introduced a 4th argument to the attach API, and this caused hangs when and older JVMs tried to attach. That was fixed by JDK-8219721, and seemed to revert the arg count back to 3, although it's not too clear to me how this was accomplished. The webrev is here: http://cr.openjdk.java.net/~xiaofeya/8219721/webrev.01/ Also at the same time the following was filed, which has not yet been addressed: JDK-8219896 Parameter size mismatch between client and VM sides of the Attach API @linzang JDK-8215622 and JDK-8219721 were your CRs, so maybe you can elaborate on them a bit more. |
Dear @plummercj, Then it comes to this patch, with "parallel" added to jmap -dump, there can be 4 arguments and we need to handle the arg count limitation. Here are the experiments I have been tried recently (and also previously when handling JDK-8219721):
This solution seems work, so the arg_count_max could be enlarged to 4, and when old jmap is targeting on new JDK, the socket can be timeout on waiting for the 4th arguments while old jmap only provide 3. And then it could work as expected. But this seems need to change the socket for different platform, and I just verified it on Linux, I am not sure whether it is acceptable.
BTW, Do you think we need to solve this issue seperately? Maybe have a patch for JDK-8219896? And when it is fixed, appling the parallel dump patch on top of that? Thanks, |
Hi Lin, |
Dear Serguei, BRs, |
Dear All, BRs, |
Ah, now the fix makes a lot more sense. I was always wondering how changing the order allowed reducing the max args to 3, but apparently the two changes are not related. Also, it seems them that JDK-8219721 does not have a correct analysis of the issue. It says:
Perhaps it should be updated to avoid future confusion. |
I think if you can avoid the 4th argument by just enabling parallel by default sounds like a good idea. Is there any reason not to use parallel heap dump? Also, I'm not familiar with the terms "compression backends" and "active workers". Can you explain them. |
For parallel heap dump, I think expose the "parallel" option is useful when user want to control the number of threads that works on parallel dumping. One reason is to control the CPU load to not affect too much on other processes on system. The "active workers" is the value that returned by gang->active_workers(). And I think "active workers" is the number of available threads that could be used for parallally working on something. my understanding is that this value could be dynamically changed at runtime, but it is limited by the JVM option "-XX:ParallelGCThreads". The "compression backend" is introduced by "gz=" option, it is actually a "backend" that write heap dump data to file, and if there is compression level specified, the "backend" will do compression first and then write to file. The whole "backend" works parallelly, before this PR, the backend thread number is dicided by "active workers". With this PR, the relation of parallel heap iteration threads (parallel dumper), the file writing threads (compression backend) and the "active_works" is like following: BRs, |
It looks like at most there would be one conflict, and it is trivial. You'll eventually have to merge before integrating anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change seems OK so far, but I've found a few issues.
Hi Ralf @schmelter-sap , BRs, |
rebased with latest master, and made some change to fix the conflict issue. I will handle Ralf's comments asap. Thanks. |
|
Hi Ralf @schmelter-sap, BRs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks @schmelter-sap and @plummercj for your help review and approve this PR! |
/integrate |
Going to push as commit a74c099.
Your commit was automatically rebased without conflicts. |
@linzang this change is causing test crashes on Windows with ZGC and EpsilonGC - please see JDK-8274196 |
Thanks David @dholmes-ora, BRs, |
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2261/head:pull/2261
$ git checkout pull/2261
Update a local copy of the PR:
$ git checkout pull/2261
$ git pull https://git.openjdk.java.net/jdk pull/2261/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2261
View PR using the GUI difftool:
$ git pr show -t 2261
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2261.diff