Skip to content
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

JDK-8318636: Add jcmd to print annotated process memory map #16301

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented Oct 22, 2023

Analysts and supporters often use /proc/xx/maps to make sense of the memory footprint of a process.

Interpreting the memory map correctly can help when used as a complement to other tools (e.g. NMT). There even exist tools out there that attempt to annotate the process memory map with JVM information.

That, however, can be more accurately done from within the JVM, at least for mappings originating from hotspot. After all, we can correlate the mapping information in NMT with the VMA map.

Example output (from a spring petstore run):

example_system_map.txt

This patch adds the VM annotations for VMAs that are mmaped. I also have an experimental patch that works with malloc'ed memory, but it is not ready yet for consumption and I leave that part of for now.


Progress

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

Issue

  • JDK-8318636: Add jcmd to print annotated process memory map (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16301

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 22, 2023

👋 Welcome back stuefe! 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
Copy link

openjdk bot commented Oct 22, 2023

@tstuefe The following labels will be automatically applied to this pull request:

  • hotspot-runtime
  • serviceability

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.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org labels Oct 22, 2023
@tstuefe tstuefe marked this pull request as ready for review October 22, 2023 19:24
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 22, 2023
@mlbridge
Copy link

mlbridge bot commented Oct 22, 2023

@plummercj
Copy link
Contributor

The hs_err file includes a dump of the maps file. It seems these annotations would be useful there also.

@tstuefe
Copy link
Member Author

tstuefe commented Oct 23, 2023

The hs_err file includes a dump of the maps file. It seems these annotations would be useful there also.

I thought about this too. The problem is the printing may take long since it is O^2 (iterate through VMAs, for each one iterate NMT regions). We had cases where the process ran with >20 mio VMAs (ZGC with a 5TB heap).

But thinking further, this is a problem anyway, since the hs-err file was >2GB. And we do have per-step timeouts in the hs-err writer. So, stopping after a set number of VMAs (lets say 100k, that covers 99% of all processes) and otherwise relying on the hs-err timeout code may be ok.

I'll give it a shot.

@tstuefe
Copy link
Member Author

tstuefe commented Oct 23, 2023

@plummercj on second thought, I'd rather leave the hs-err part for a follow-up RFE.

@plummercj
Copy link
Contributor

@plummercj on second thought, I'd rather leave the hs-err part for a follow-up RFE.

That's fine. Thanks for considering it.

@zhengyu123
Copy link
Contributor

The hs_err file includes a dump of the maps file. It seems these annotations would be useful there also.

I thought about this too. The problem is the printing may take long since it is O^2 (iterate through VMAs, for each one iterate NMT regions). We had cases where the process ran with >20 mio VMAs (ZGC with a 5TB heap).

But thinking further, this is a problem anyway, since the hs-err file was >2GB. And we do have per-step timeouts in the hs-err writer. So, stopping after a set number of VMAs (lets say 100k, that covers 99% of all processes) and otherwise relying on the hs-err timeout code may be ok.

I'll give it a shot.

IIRC, both lists are sorted, should be a way to reduce time complexity.

@openjdk
Copy link

openjdk bot commented Oct 26, 2023

@tstuefe this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8318636-Add-jcmd-to-print-annotated-process-memory-map
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Oct 26, 2023
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Oct 26, 2023
Copy link
Contributor

@jdksjolen jdksjolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thank you for this PR. These comments are just a first pass, I haven't finished going through the code.

src/hotspot/os/linux/memMapPrinter_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/memMapPrinter_linux.cpp Show resolved Hide resolved
src/hotspot/share/services/memMapPrinter.hpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/memMapPrinter_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/memMapPrinter_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/memMapPrinter_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/memMapPrinter_linux.cpp Outdated Show resolved Hide resolved
@tstuefe
Copy link
Member Author

tstuefe commented Oct 27, 2023

@jdksjolen Many thanks for the review. I fixed most of your requests, and also added a simple timeout fuse to prevent the printing from taking overly long in the rare case of insanely large or fragmented process memory maps.

@gerard-ziemski
Copy link

Taking a look...

Copy link

@gerard-ziemski gerard-ziemski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this feature, hope the other platforms can be done as well. (I am also really looking forward to seeing how you did the "malloc" version)

I think we should set the user expectations correctly and say somewhere that these mappings are only for mmaped memory, ex change:

Memory mappings:

to

Memory mappings (mmaped):

or something like that.

These lines look akward next to each other:

0x000000070a400000 - 0x0000000800000000  4123000832  ---p 00000000    JAVAHEAP                      
0x00007f4e747dd000 - 0x00007f4e747e1000       16384  ---p 00000000    STACK(72821 "AWT-EventQueue-0") 
0x0000563e6decb000 - 0x0000563e6decc000        4096  r--p 00000000                                  /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/bin/java
0x00007f4e26000000 - 0x00007f4e26d3b000    13873152  rw-p 00001000    CDS                           /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/lib/server/classes.jsa

in my opinion. The majority of entries either have the component or path in the info field (except CDS). Could we print it simply as:

0x000000070a400000 - 0x0000000800000000  4123000832  ---p 00000000    JAVAHEAP                      
0x00007f4e747dd000 - 0x00007f4e747e1000       16384  ---p 00000000    STACK(72821 "AWT-EventQueue-0") 
0x0000563e6decb000 - 0x0000563e6decc000        4096  r--p 00000000    /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/bin/java
0x00007f4e26000000 - 0x00007f4e26d3b000    13873152  rw-p 00001000    CDS=/home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/lib/server/classes.jsa

I find the compact way more aesthetically pleasing and easier to read.

Next I'm going to start looking into the implementation...

src/hotspot/share/services/memMapPrinter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/services/memMapPrinter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/services/memMapPrinter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/services/memMapPrinter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/services/memMapPrinter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/services/memMapPrinter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/services/memMapPrinter.cpp Outdated Show resolved Hide resolved
@tstuefe
Copy link
Member Author

tstuefe commented Oct 28, 2023

I really like this feature, hope the other platforms can be done as well. (I am also really looking forward to seeing how you did the "malloc" version)

I think we should set the user expectations correctly and say somewhere that these mappings are only for mmaped memory, ex change:

Memory mappings:

to

Memory mappings (mmaped):

or something like that.

That would be more confusing since you can see C-heap mappings as well (mappings the libc allocates via mmap to back its arenas). We don't have VM annotations for them (yet), but they do show up in that list.

These lines look akward next to each other:

0x000000070a400000 - 0x0000000800000000  4123000832  ---p 00000000    JAVAHEAP                      
0x00007f4e747dd000 - 0x00007f4e747e1000       16384  ---p 00000000    STACK(72821 "AWT-EventQueue-0") 
0x0000563e6decb000 - 0x0000563e6decc000        4096  r--p 00000000                                  /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/bin/java
0x00007f4e26000000 - 0x00007f4e26d3b000    13873152  rw-p 00001000    CDS                           /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/lib/server/classes.jsa

in my opinion. The majority of entries either have the component or path in the info field (except CDS). Could we print it simply as:

0x000000070a400000 - 0x0000000800000000  4123000832  ---p 00000000    JAVAHEAP                      
0x00007f4e747dd000 - 0x00007f4e747e1000       16384  ---p 00000000    STACK(72821 "AWT-EventQueue-0") 
0x0000563e6decb000 - 0x0000563e6decc000        4096  r--p 00000000    /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/bin/java
0x00007f4e26000000 - 0x00007f4e26d3b000    13873152  rw-p 00001000    CDS=/home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/lib/server/classes.jsa

Hmm. Yes, that looks better. I was originally worried about VMAs for files being folded with other ones, then you would have confusing entries like "STACK.. filename bla". But that can actually never happen since VMAs with different files (or one has ha file, one don't) are not folded. Yes, I change that.

I find the compact way more aesthetically pleasing and easier to read.

Next I'm going to start looking into the implementation...

Thansk!

@tstuefe
Copy link
Member Author

tstuefe commented Oct 28, 2023

@gerard-ziemski @jdksjolen I take this back to draft for the moment since I found that I need to solve at least two additional problems.

One is the time it takes to correlate NMT regions with VMA - its O^2, and experiments with millions of mappings show that this does not scale well. My new plan is to pre-collect all NMT information available first in a flat cache-friendly array that can be iterated cheaply, possibly pre-sorted, and use that when iterating VMAs. And only do the first part in a VM Op.

The second problem is that the output can get huge and I run against limits in dcmd framework. I may just do what other commands do, dump to a file instead to the output.

@tstuefe tstuefe marked this pull request as draft October 28, 2023 07:39
@tstuefe
Copy link
Member Author

tstuefe commented Nov 2, 2023

LGTM

Thank you for doing this!

Thank you for reviewing, @gerard-ziemski !

@tstuefe
Copy link
Member Author

tstuefe commented Nov 3, 2023

Windows GHA errors unrelated.

@tstuefe
Copy link
Member Author

tstuefe commented Nov 4, 2023

@jdksjolen anything more needed from my side?

@jdksjolen
Copy link
Contributor

@jdksjolen anything more needed from my side?

I'll work through this tomorrow, just to make sure I didn't miss anything.

@jdksjolen
Copy link
Contributor

As this adds a JCmd, doesn't this need both a CSR and a manual entry?

@tstuefe
Copy link
Member Author

tstuefe commented Nov 8, 2023

As this adds a JCmd, doesn't this need both a CSR and a manual entry?

  • CSR: not sure; there are precedences for going with CSR and without CSR. Since we get awfully close to JDK22 freeze, I would prefer for a CSR not to be needed. Also would make backporting a lot easier.
  • Manpage: not sure either; IIRC last time I tried, I was told that Oracle maintains these internally, because there is internal documentation that needs updating too?

@jdksjolen
Copy link
Contributor

As this adds a JCmd, doesn't this need both a CSR and a manual entry?

* CSR: not sure; there are precedences for going with CSR and without CSR. Since we get awfully close to JDK22 freeze, I would prefer for a CSR not to be needed. Also would make backporting a lot easier.

* Manpage: not sure either; IIRC last time I tried, I was told that Oracle maintains these internally, because there is internal documentation that needs updating too?

@dholmes-ora, would you mind helping us out here with regards to the process? Would a new JCmd require a CSR or would it be acceptable to merge without one? Thank you.

@dholmes-ora
Copy link
Member

As this adds a JCmd, doesn't this need both a CSR and a manual entry?

* CSR: not sure; there are precedences for going with CSR and without CSR. Since we get awfully close to JDK22 freeze, I would prefer for a CSR not to be needed. Also would make backporting a lot easier.

* Manpage: not sure either; IIRC last time I tried, I was told that Oracle maintains these internally, because there is internal documentation that needs updating too?

@dholmes-ora, would you mind helping us out here with regards to the process? Would a new JCmd require a CSR or would it be acceptable to merge without one? Thank you.

@jdksjolen No CSR needed: from another related PR - "We do not use CSR requests with jcmd changes as they are deemed diagnostic commands - ref JDK-8203682"

But yes the jcmd manpage should be updated ref:
https://docs.oracle.com/en/java/javase/19/docs/specs/man/jcmd.html
though I'm worried we may not have kept it up to date! That requires an Oracle engineer to apply the changes to the jcmd.md markdown sources in our repo, and then regenerate the jcmd.1 manpage file. The doc update can be split into a separate doc sub-task so that the main PR is not held up. (And we probably need an audit to see if any other updates are missing - which is painful.)

@tstuefe
Copy link
Member Author

tstuefe commented Nov 9, 2023

As this adds a JCmd, doesn't this need both a CSR and a manual entry?

* CSR: not sure; there are precedences for going with CSR and without CSR. Since we get awfully close to JDK22 freeze, I would prefer for a CSR not to be needed. Also would make backporting a lot easier.

* Manpage: not sure either; IIRC last time I tried, I was told that Oracle maintains these internally, because there is internal documentation that needs updating too?

@dholmes-ora, would you mind helping us out here with regards to the process? Would a new JCmd require a CSR or would it be acceptable to merge without one? Thank you.

@jdksjolen No CSR needed: from another related PR - "We do not use CSR requests with jcmd changes as they are deemed diagnostic commands - ref JDK-8203682"

But yes the jcmd manpage should be updated ref: https://docs.oracle.com/en/java/javase/19/docs/specs/man/jcmd.html though I'm worried we may not have kept it up to date! That requires an Oracle engineer to apply the changes to the jcmd.md markdown sources in our repo, and then regenerate the jcmd.1 manpage file. The doc update can be split into a separate doc sub-task so that the main PR is not held up. (And we probably need an audit to see if any other updates are missing - which is painful.)

Would be nice if that process were open like the rest. It would take load from Oracle and most developers would be fine with maintaining the man page themselves.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the command is only usable on Linux, and only registered on Linux, do we really need the empty definitions in memMapPrinter_<os>.cpp? At present it seems we only need it because all the implementation code is not isolated to ifdef LINUX.

SystemMapDCmd(outputStream* output, bool heap);
static const char* name() { return "System.map"; }
static const char* description() {
return "Prints an annotated process memory map of the VM process.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this specify "Linux Only" ?

@@ -471,6 +471,7 @@ void Thread::print_on(outputStream* st, bool print_extended_info) const {
if (!is_Java_thread() || !JavaThread::cast(this)->is_vthread_mounted()) {
osthread()->print_on(st);
}
st->print("stack=[" PTR_FORMAT ", " PTR_FORMAT ")", p2i(stack_end()), p2i(stack_base()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already report the thread stack elsewhere, so is this going to duplicate the information?

SystemDumpMapDCmd(outputStream* output, bool heap);
static const char* name() { return "System.dump_map"; }
static const char* description() {
return "Dumps an annotated process memory map to an output file.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again "Linux Only"?

@@ -117,7 +117,8 @@
template(GTestStopSafepoint) \
template(JFROldObject) \
template(JvmtiPostObjectFree) \
template(RendezvousGCThreads)
template(RendezvousGCThreads) \
template(SystemMap)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't spot the VMop for this. ??

name = absname != nullptr ? absname : name;
output()->print_cr("Memory map dumped to \"%s\".", name);
} else {
output()->print_cr("Failed to open \"%s\" for writing.", name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you want to report errno here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always leery about relying on errno from wrapper functions or objects like this, unless they have an explicit channel to report the relevant errno. I looked at fileStream, I guess its okay here. Okay, I'll add it.

@tstuefe
Copy link
Member Author

tstuefe commented Nov 10, 2023

@dholmes-ora thank you for your review. I hope I have addressed all concerns. I made this all Linux only, removed os::realpath, print errno.

I am currently working on a follow-up RFE that provides this command for Windows, and it works, but I will do it in a separate RFE (rolling back the ifdef LINUX should be easy) and possibly not for JDK 22. #16593

Copy link
Contributor

@jdksjolen jdksjolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Thomas,

Thanks for your hard work on this! LGTM.

@openjdk
Copy link

openjdk bot commented Nov 10, 2023

@tstuefe 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:

8318636: Add jcmd to print annotated process memory map

Reviewed-by: jsjolen, gziemski

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 97 new commits pushed to the master branch:

  • 9cce9fe: 8319256: Print more diagnostic information when an unexpected user is found in a Phi
  • a95062b: 8319670: Improve comments describing system properties for TLS server and client for max chain length
  • 38745ec: 8319649: inline_boxing_calls unused gvn variable
  • 636a351: 8319429: Resetting MXCSR flags degrades ecore
  • d7b0ba9: 8319554: Select LogOutput* directly for stdout and stderr
  • 68110b7: 8319574: Exec/process tests should be marked as flagless
  • 7b971c1: 8319705: RISC-V: signumF/D intrinsics fails compiler/intrinsics/math/TestSignumIntrinsic.java
  • f939542: 8319324: FFM: Reformat javadocs
  • a3f1b33: 8319664: IGV always output on PhaseRemoveUseless
  • f57b78c: 8319726: Parallel GC: Re-use object in object-iterator
  • ... and 87 more: https://git.openjdk.org/jdk/compare/2d4a4d04b876a8da5fa6c962911d36d547f214fe...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2023
@tstuefe
Copy link
Member Author

tstuefe commented Nov 10, 2023

Hi Thomas,

Thanks for your hard work on this! LGTM.

Thank you for your Review, Johan!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @tstuefe - nothing further from me (with the Windows variant in the pipeline I will wait and see how things look in terms of shared versus platform-specific code.

Please create a JBS issue to get this new command documented in the jcmd manpage, with details on what that documentation should look like.

Thanks

@tstuefe
Copy link
Member Author

tstuefe commented Nov 13, 2023

Thanks @dholmes-ora !

/integrate

Thanks for the updates @tstuefe - nothing further from me (with the Windows variant in the pipeline I will wait and see how things look in terms of shared versus platform-specific code.

Please create a JBS issue to get this new command documented in the jcmd manpage, with details on what that documentation should look like.

There are a number of inconsistencies apart from this new dcmd; I think the jcmd man page needs to be updated.

I collected all inconsistencies I could spot in this issue: https://bugs.openjdk.org/browse/JDK-8319948 and put it onto your name.

Thanks

Thank you, David.

@openjdk
Copy link

openjdk bot commented Nov 13, 2023

Going to push as commit 6f863b2.
Since your change was applied there have been 106 commits pushed to the master branch:

  • e035637: 8319375: test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java runs into OutOfMemoryError: Metaspace on AIX
  • 50f41d6: 8309893: Integrate ReplicateB/S/I/L/F/D nodes to Replicate node
  • caf7181: 8318189: ChoiceFormat::format throws undocumented AIOOBE
  • 9938b3f: 8319314: NMT detail report slow or hangs for large number of mappings
  • c9077b8: 8319339: Internal error on spurious markup in a hybrid snippet
  • ea1ffa3: 8318895: Deoptimization results in incorrect lightweight locking stack
  • c9657ca: 8319882: SequenceLayout::toString throws ArithmeticException
  • 6b21ff6: 8319828: runtime/NMT/VirtualAllocCommitMerge.java may fail if mixing interpreted and compiled native invocations
  • a64fc48: 8319174: Enhance robustness of some j.m.BigInteger constructors
  • 9cce9fe: 8319256: Print more diagnostic information when an unexpected user is found in a Phi
  • ... and 96 more: https://git.openjdk.org/jdk/compare/2d4a4d04b876a8da5fa6c962911d36d547f214fe...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 13, 2023
@openjdk openjdk bot closed this Nov 13, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 13, 2023
@openjdk
Copy link

openjdk bot commented Nov 13, 2023

@tstuefe Pushed as commit 6f863b2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
7 participants