8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set #18190
Conversation
|
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
/label add serviceability |
|
@plummercj |
|
/label add hotspot-gc GC folk should be reviewing this not runtime. |
|
@dholmes-ora |
I don't fully agree with that. How these serviceability tools work, and their interfaces, are usually not something that we GC devs are directly responsible for. This change could be reviewed by any HotSpot developer that has a stake in the hprof tooling. |
There was a problem hiding this comment.
-XX:HeapDumpPath was introduced by -XX:+HeapDumpOnOutOfMemoryError for its use.
globals.hpp documents HeapDumpPath as relating to HeapDumpOnOutOfMemoryError (so that will need changing if this change is happening).
It does look useful to use HeapDumpPath as the default for jcmd, although maybe there's some confusion possible if you're already relying on it for OOM dumps.
jcmd GC.heap_dump has the overwrite flag.
OOM dumping in HeapDumper::dump_heap(bool oome) has a sequence number.
Can it be specified how these will interact? 8-)
Does overwrite mean you don't want to increase sequence number (but we already increased it last iteration)...
I like splitting out alloc_and_create_heapdump_pathname() as this is already a large part of dump_heap.
Should the comment say, "caller must free the returned pointer".
HeapDumper::dump_to_heapdump_path
This new name makes less sense to me, as we already use heapdump path, but maybe you need it to avoid the dcmd having to work out the final filename. 8-)
Does dump_to_heapdump_path() not print the ("Dumping heap to %s ...", path) message? That seems important when the user isn't specifying it directly.
But the policy questions probably important to confirm before worrying about those and any other code comments.
A test would not be a bad thing. 8-)
|
Hi Kevin, thanks for the comments.
Yes true, probably we have to adjust the related text. old "When HeapDumpOnOutOfMemoryError is on," ? |
There is (with this patch) still just one sequence number and it is incremented by all invocations of alloc_and_create_heapdump_pathname. |
Agree, I will adjust the comment. (and btw regarding your comment on a test, yes I agree there should be a separate test or at least an adjustment/addition to the existing tests) |
The path is already printed, here is an example (the JVM with pid 219447 was started with images/jdk/bin/jcmd 219447 GC.heap_dump |
yes of course, when the new method calls dump(), thanks. |
Right, I was trying to say that previously, sequence numbers were only used by OOM dumping, and now jcmd invocation can use them, if you have it using HeapDumpPath. (But it can't when using a specified filename, that might be a point of confusion.) HeapDumpOnOutOfMemoryError dumping never overwrites as I read it. That make sense as it could overwrite existing dumps if an OOM happens, and means a sequence of dumps should all have the same origin. Invoked from jcmd with HeapDumpPath and -overwrite, can you can overwrite part of a sequence. |
Thanks for clarifying, maybe we have to describe/document this at some place (comment? or jcmd help ?) . |
Sorry I assumed GC folk had an interest in |
|
❗ This change is not yet ready to be integrated. |
Do you maybe have a good suggestion for a new name ? |
|
Hi Kevin,
I adjusted the related text in globals.hpp . Please check. |
|
Hi Kevin,
I added a test HeapDumpJcmdPresetPathTest.java . |
|
Notwithstanding that there may be cases where this change would be convenient, I really don't like this coupling between the jcmd behaviour and a -XX flag that is intended for something else. It doesn't completely mesh with the jcmd usage and other options, and the documentation story is quite complicated.
To me that is not a sufficient benefit for the complexity this change would add. Further, it is not at all clear to me that a dynamic heap-dump should be using the same destination as that set for the |
You would file a JBS issue with component "docs" and subcategory "hotspot". But let's hold off on that for now until if it is decided if the PR is going to be pushed. My main reason for mentioning it was to point out additional fallout of this change.
Backporting also came to mind since the trouble shooting guide would need to be updated for each Oracle version this PR is backported to. And yes, HeapDumpGzipLevel docs need to match the actual implementation. I'm not sure if HeapDumpGzipLevel has been backported to 8, 11, and 17 (in the open for for the Oracle release). I'm inclined to say a separate jbs issue should track updating docs for HeapDumpGzipLevel, but then we also have the question of whether or not HeapDumpGzipLevel should impact the jcmd if this PR is to be pushed. |
I did get feedback from a couple of our support engineers. One felt it was of no real benefit as it was just as easy to provide the filename as an argument to the jcmd. The other thought it might be of some benefit, but also had a misunderstanding of how the jcmd currently works (thought that with no filename given, it would automatically dump into the current directory). There were no strong arguments for this PR, especially that overcome all the issues it creates. |
|
To weight in on the side of this patch, in most customer scenarios I have seen, there is just one location for heap dumps. Using the same location for OOM and for manual heap dumps seems logical to me.
Wouldn't this just be a case of changing a flag description? As luck has it, the flag already has a generic name that is not tied to OOMs. |
HeapDumpGzipLevel is in openjdk17 and openjdk21 . |
That might be true for this support engineer, but not for others with other setups where it is an issue to find out where to write in cloud/container scenarios . |
Yes it is some work on documentation (but seems some doc work needs to be done anyway because it was forgotten when |
One of the issues with this PR is that there are 7 places where some sort of doc/help update is needed. The help output for the
And even this is not complete. HeapDumpPath can be a directory, in which case a filename is generated using the PID and a dump sequence number. Probably all this belongs in the argument description, which currently just says "Name of dump file", but you still need to put something in for the default. If the description is complete, maybe just using "See description" would be better. |
That currently only impacts 3 documents. Having HeapDumpGzipLevel apply to the jcmd impacts 4 more, and if it doesn't apply to the jcmd then you have an inconsistency with how HeapDumpXXX options are applied. There's also a question of whether currently missing doc updates for HeapDumpGzipLevel should be made part of this PR because it complicates back porting. We need to make sure we don't backport HeapDumpGzipLevel changes to a JDK version that has HeapDumpPath but not HeapDumpGzipLevel. I think as of right now HeapDumpPath is in 11 but HeapDumpGzipLevel is only backported to 17.
There's still the question of whether or not it is even appropriate to have -XX options taking the place of jcmd options. |
It should most likely be a separate PR (the title of this one does not match, and there are different backporting needs). |
Some people (like our cloud support colleagues and also some who commented) would like this approach, some find it not very useful. |
|
Couldn't we just have an unconditional default for the GC.heap_dump As for the general value of this feature, I think it can simplify the handling of certain (cloud) deployment configurations where you have a dedicated filesystem that you want to write these kind of dumps to. It might also be nice if there was a default value for filename instead of the obligation to specify something. And, for those who can't see the upside of it all, I at least can't see no harm... |
Then you end up with the same issue that this PR has with 7 docs to update (and the "descriptive" default for the jcmd). I'm not advocating one approach over the other. I'm just pointing out that either approach has its warts, and they are rooted in the changes done for this PR. |
That's a little different. For some reason (I'm not sure of the details) it was decided to not allow a dynamic dump with VM.cds unless it was explicitly enabled on the command line with the -XX:+ RecordDynamicDumpInfo option. This is different than having a jvm option that is used in place of an omitted jcmd option. |
I've had similar thoughts while considering this PR. I'm not sure why currently there is no default. HeapDumpPath is not quite as simple as you document it above. It can be a filename or a directory, and when it is a directory it has its own default file naming. This should be captured in the jcmd help. |
|
I disagree with the premise of this entire request that jcmd GC.heap_dump should follow -XX:HeapDumpPath. I've been testing this on on JDK 8 and JDK 21 and confirmed in both versions that jcmd requires the filename argument that must be a file, not a directory. Supplying just a directory will fail with either "file exists" or "is a directory", and supplying just a filename without a preceding directory will create the dump in the current working directory (cwd). Currently, the documentation isn't clear that filename is required. This should be corrected and also suggest to add %p and %d to the filename, which is useful when you don't want them overwritten with the -overwrite option (JDK 21) or when running heap dumps on multiple processes at once (with a 0 pid or with main-class specified instead of pid). Regarding this justification:
It's too weak an argument to justify such a change. Simple scripts can capture and propagate the HeapDumpPath value, if you really want to go that route. However, I recommend using separate directories for manual heap dumps. Keeping them separate from JVM-initiated heap dumps, either through filename convention or separate directories, is a best practice. When you have a severity 1 down situation due to OOME crashes, you don't want to have a bunch of manually pulled heap dumps cluttering up your view of the JVM initiated ones. You want to be able to get your hands on the exact heap dumps you need very quickly to have them analyzed for root cause. For this reason I also disagree with setting a default filename convention equivalent to the default HeapDumpPath filename. Manual heap dumps require specifying a filename for a reason, and those filenames (and separate directories for those who define them) should be unique based on what triggered the heap dump. Consider the numerous Support cases of multiple monitoring scripts kicking off jcmd processes to pull heap dumps at various resource usage thresholds, as well as multiple Java agents that may do so, e.g. for various performance reasons. |
With this change it is still possible to have separation (nobody stops users from using jcmd with the filename option, this is still available). |
|
Yes, exactly the latter. With this change, it opens the door for the above scenario to unwittingly occur, either through user error (simple omission of a previously used parameter) or lack of understanding of the impact not using the parameter will have. |
|
Hi @spayne8, thanks for your elaborate answer. Let me comment on your points:
I at least can't see the regression here, because it's still possible to separate the locations of automatic and manual heap dumps by setting a value to the jcmd call.
Sure but we want to avoid that need and have it more comfortable without having to write scripts.
Sure, but the change requested here does not go against that best practice. You still can have different location for manual heap dumps and the file systems should probably cleaned up from old heap dumps anyway.
Well, I guess, after all it's a bit of a matter of taste. I personally would think having a default for hprof files obtained by jcmd should provide some kind of comfort. Maybe/probably that default should be different from the one for the JVM initiated automatic heapdumps but still it would be helpful if it could be configured, maybe by another XX option. |
|
I don't have strong opinion if it's good to have default file path for
Current patch looks inconsistent: |
Was that ever a problem raised by users that the OOME and the GC related heap dumps end up at the same location ?
Yes , probably with 1. + 2, and now (with the patch) 3. using HeapDumpPath the others should use it most likely too. |
I could open a separate JBS issue (or issues) for scenario 4 and 5 , if this is desired. |
|
One other thing I just realized is that if we go through with this change, then we can't in the future change VM.heap_dump so it can be used without any filename (make it use a default filename when no filename is specified). Setting HeapDumpPath would override this default filename, and it would be really confusing to the user when suddenly heap dumps are not appearing in the default location with the default name because someone decided to specify HeapDumpPath when launching the JVM. |
Isn't this the case already for the -XX:+HeapDumpBeforeFullGC, -XX:+HeapDumpAfterFullGC scenarios? |
I don't think so, but maybe I'm misunderstanding your point. It may be desirable to have VM.heap_dump support not specifying a filename and instead just have it choose a default name and path, probably the current working directory with a name something like java_pid.hprof. However, having this default and also using HeapDumpPath if set conflict with each other. Someone could be relying on the VM.heap_dump default, and then suddenly that default is no longer honored because HeapDumpPath was added to the command line. I don't see -XX:+HeapDumpBeforeFullGC or -XX:+HeapDumpAfterFullGC playing a roll in this scenario. |
openjdk
bot
changed the title
So is there some intention to decide to have such a default now or in the near future ? |
No decision on having a default has been made. My point was that implementing this PR equates to making a decision NOT to ever support a default. It doesn't leave it to be decided some time in the future. |
|
Another option would be to implement an additional jcmd command where jcmd GC.heap_dump_other_command_name without options writes to location given by -XX:HeapDumpPath, if set. This would avoid all the compatibility concerns raised by changing jcmd GC.heap_dump; would this be a welcome change ? |
|
That might be acceptable, although coming up with a good name will be a challenge. |
Currently jcmd command GC.heap_dump only works with an additionally provided file name.
Syntax : GC.heap_dump [options]
In case the JVM has the XX - flag HeapDumpPath set, we should support an additional mode where the is optional.
In case the filename is NOT set, we take the HeapDumpPath (file or directory);
new syntax :
GC.heap_dump [options] .. has precedence over second option
GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set
This would be a simplification e.g. for support cases where a filename or directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the path is intended/recommended for usage also in the jcmd case.
Progress
Warning
8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if setIssue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18190/head:pull/18190$ git checkout pull/18190Update a local copy of the PR:
$ git checkout pull/18190$ git pull https://git.openjdk.org/jdk.git pull/18190/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18190View PR using the GUI difftool:
$ git pr show -t 18190Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18190.diff
Webrev
Link to Webrev Comment