8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set

[MBaesken]

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

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [ ] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Warning

 ⚠️ Found leading lowercase letter in issue title for 8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set

Issue

• JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set (Bug - P4)

Reviewers

• Andreas Steiner (@ansteiner - Author) ⚠️ Review applies to 61165f55

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18190/head:pull/18190
$ git checkout pull/18190

Update a local copy of the PR:
$ git checkout pull/18190
$ git pull https://git.openjdk.org/jdk.git pull/18190/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18190

View PR using the GUI difftool:
$ git pr show -t 18190

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18190.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@MBaesken The following label will be automatically applied to this pull request:

• hotspot-runtime

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.

[mlbridge[bot]]

Webrevs

• 09: Full - Incremental (8039b9a4)
• 08: Full - Incremental (b414b1be)
• 07: Full - Incremental (8b48c911)
• 06: Full - Incremental (a71f04b0)
• 05: Full - Incremental (b97921d5)
• 04: Full - Incremental (daab7df9)
• 03: Full - Incremental (61165f55)
• 02: Full - Incremental (7420fb47)
• 01: Full - Incremental (e335d9ee)
• 00: Full (116c0572)

[plummercj]

/label add serviceability

[openjdk[bot]]

@plummercj The serviceability label was successfully added.

[dholmes-ora]

/label add hotspot-gc

GC folk should be reviewing this not runtime.

[openjdk[bot]]

@dholmes-ora The hotspot-gc label was successfully added.

[stefank]

GC folk should be reviewing this not runtime.

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.

[MBaesken]

Hi Kevin, thanks for the comments.

globals.hpp documents HeapDumpPath as relating to HeapDumpOnOutOfMemoryError

Yes true, probably we have to adjust the related text. What do you think about this

old "When HeapDumpOnOutOfMemoryError is on," new "When HeapDumpOnOutOfMemoryError is on or a heap dump is trigger by jcmd GC.heap_dump without specifying a path,"

?

[MBaesken]

jcmd GC.heap_dump has the overwrite flag. OOM dumping in HeapDumper::dump_heap(bool oome) has a sequence number.

There is (with this patch) still just one sequence number and it is incremented by all invocations of alloc_and_create_heapdump_pathname.

[MBaesken]

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

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)

[MBaesken]

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.

The path is already printed, here is an example (the JVM with pid 219447 was started with-XX:HeapDumpPath=... ).

images/jdk/bin/jcmd 219447 GC.heap_dump 219447: Dumping heap to /mydir/test/dumpdir/dydumpfile ... Heap dump file created [3757116 bytes in 0.046 secs]

[kevinjwalls]

The path is already printed, here is an example (the JVM with pid 219447 was started with-XX:HeapDumpPath=... ).

yes of course, when the new method calls dump(), thanks.

[kevinjwalls]

jcmd GC.heap_dump has the overwrite flag. OOM dumping in HeapDumper::dump_heap(bool oome) has a sequence number.

There is (with this patch) still just one sequence number and it is incremented by all invocations of alloc_and_create_heapdump_pathname.

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. That's not necessarily wrong, and may not be serious enough to refuse -overwrite when using HeapDumpPath, but we want to find a way of making things clear (I was not going to suggest separate sequence numbers... 8-) )

[MBaesken]

That's not necessarily wrong, and may not be serious enough to refuse -overwrite when using HeapDumpPath, but we want to find a way of making things clear (I was not going to suggest separate sequence numbers... 8-) )

Thanks for clarifying, maybe we have to describe/document this at some place (comment? or jcmd help ?) .

[dholmes-ora]

GC folk should be reviewing this not runtime.

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.

Sorry I assumed GC folk had an interest in -XX:HeapDumpPath and so how it might get used.

[openjdk[bot]]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

[MBaesken]

Looks reasonable, this is a harmless change, but the name dump_to_heapdump_path looks too details(and somewhat strange) to me

Do you maybe have a good suggestion for a new name ?

[MBaesken]

Hi Kevin,

globals.hpp documents HeapDumpPath as relating to HeapDumpOnOutOfMemoryError (so that will need changing if this change is happening).

I adjusted the related text in globals.hpp . Please check.

[MBaesken]

Hi Kevin,

(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)

I added a test HeapDumpJcmdPresetPathTest.java .

[y1yang0]

Looks reasonable, this is a harmless change, but the name dump_to_heapdump_path looks too details(and somewhat strange) to me

Do you maybe have a good suggestion for a new name ?

Maybe dump and dump_to

[MBaesken]

Maybe dump and dump_to

Why not, I renamed the method to dump_to .

[MBaesken]

Hi Chris, thanks for the comments-

Also, if you are cleaning up this text, I would suggest changing "is on" to "is enabled". Same for HeapDumpGzipLevel below.

I changed the two locations. btw. seems we have more of those in globals.hpp . See the comments related to PrintNMTStatistics and LogFile (where the "is on" is used as well).

[plummercj]

Hi Chris, thanks for the comments-

Also, if you are cleaning up this text, I would suggest changing "is on" to "is enabled". Same for HeapDumpGzipLevel below.

I changed the two locations. btw. seems we have more of those in globals.hpp . See the comments related to PrintNMTStatistics and LogFile (where the "is on" is used as well).

I didn't realize there were other occurrences. Up to you if you want to change some, all, or none.

[dholmes-ora]

Once the changes to the jcmd manpage are finalized please ask someone from Oracle serviceability to apply the changes to the closed sources and regenerate the troff file to ensure things match. Thanks.

[MBaesken]

In a cloud environment using containers, the HeapDumpPath is automatically set to a file system service to persist the heapdump. However, if a support engineer or DevOps detects high or increasing memory usage and wants to trigger a heapdump via jcmd, they would need to specify the filename. This requires retrieving the set HeapDumpPath from the app environment and copying it to the jcmd to use the persistent file system. This change can help avoid the need for an additional copy and paste step, which is prone to errors.

Hi Andreas , thanks for the details . Chris, is this understable for you? We indeed had quite a few user complains by cloud env users, that the HeapDumpPath is currently not evaluated in the jcmd case/scenario .

[plummercj]

Hi Andreas , thanks for the details . Chris, is this understable for you? We indeed had quite a few user complains by cloud env users, that the HeapDumpPath is currently not evaluated in the jcmd case/scenario .

I'm still somewhat skeptical about this change and its value. The impact on help/docs and using -XX options in this manner don't sit well with me. I'd like to consult with a couple of people at Oracle who unfortunately are out of town until Monday. Can this PR wait until next week?

[MBaesken]

I'd like to consult with a couple of people at Oracle who unfortunately are out of town until Monday. Can this PR wait until next week?

That's fine, no need to rush it into jdk-head this week.

[MBaesken]

Hi Chris, thanks for the suggestions , I added them to the latest commit.

[plummercj]

We have a couple of additional documents that need to be updated:

https://docs.oracle.com/en/java/javase/21/troubleshoot/command-line-options1.html#GUID-A84ECBFB-B6CF-44C3-B627-58BB509C8D05

This is an Oracle produced document. A docs CR will need to be filed to get it updated (and I see it is already appears out-of-date w.r.t. HeapDumpGzipLevel)

src/java.base/share/man/java.1

Much like jcmd.1, this one is also derived from a closed markdown file, so eventually someone internal at Oracle will need to update the markdown file, but we need to see the proposed changes in the java.1 file first. This document is also missing the HeapDumpGzipLevel update.

If your counting, there are 7 places where documentation will need updating

1. Oracle trouble shooting guide
2. open java.1
3. closed markdown file that java.1 is generated from
4. open jcmd.1
5. closed markdown file that jcmd.1 is generated from
6. globals.hpp
7. jcmd help output