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

8254723: add diagnostic command to write Linux perf map file #760

Closed
wants to merge 5 commits into from

Conversation

@nick-arm
Copy link
Member

@nick-arm nick-arm commented Oct 20, 2020

When using the Linux "perf" tool to do system profiling, symbol names of
running Java methods cannot be decoded, resulting in unhelpful output
such as:

10.52% [JIT] tid 236748 [.] 0x00007f6fdb75d223

Perf can read a simple text file format describing the mapping between
address ranges and symbol names for a particular process [1].

It's possible to generate this already for Java processes using a JVMTI
plugin such as perf-map-agent [2]. However this requires compiling
third-party code and then loading the agent into your Java process. It
would be more convenient if Hotspot could write this file directly using
a diagnostic command. The information required is almost identical to
that of the existing Compiler.codelist command.

This patch adds a Compiler.perfmap diagnostic command on Linux only. To
use, first run "jcmd Compiler.perfmap" and then "perf top" or
"perf record" and the report should show decoded Java symbol names for
that process.

As this just writes a snapshot of the code cache when the command is
run, it will become stale if methods are compiled later or unloaded.
However this shouldn't be a big problem in practice if the map file is
generated after the application has warmed up.

[1] https://github.com/torvalds/linux/blob/master/tools/perf/Documentation/jit-interface.txt
[2] https://github.com/jvm-profiling-tools/perf-map-agent


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8254723: Add diagnostic command to write Linux perf map file

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/760/head:pull/760
$ git checkout pull/760

When using the Linux "perf" tool to do system profiling, symbol names of
running Java methods cannot be decoded, resulting in unhelpful output
such as:

  10.52% [JIT] tid 236748 [.] 0x00007f6fdb75d223

Perf can read a simple text file format describing the mapping between
address ranges and symbol names for a particular process [1].

It's possible to generate this already for Java processes using a JVMTI
plugin such as perf-map-agent [2]. However this requires compiling
third-party code and then loading the agent into your Java process. It
would be more convenient if Hotspot could write this file directly using
a diagnostic command. The information required is almost identical to
that of the existing Compiler.codelist command.

This patch adds a Compiler.perfmap diagnostic command on Linux only. To
use, first run "jcmd <pid> Compiler.perfmap" and then "perf top" or
"perf record" and the report should show decoded Java symbol names for
that process.

As this just writes a snapshot of the code cache when the command is
run, it will become stale if methods are compiled later or unloaded.
However this shouldn't be a big problem in practice if the map file is
generated after the application has warmed up.

[1] https://github.com/torvalds/linux/blob/master/tools/perf/Documentation/jit-interface.txt
[2] https://github.com/jvm-profiling-tools/perf-map-agent
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 20, 2020

👋 Welcome back ngasson! 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.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 20, 2020

/label add serviceability

@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@nick-arm
The serviceability label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 20, 2020

Copy link
Contributor

@shipilev shipilev left a comment

That's a nifty idea! (Kicks himself for not coming up with it himself.)

I do wonder the UX would be even better if we dump the map file at VM shutdown (guarded by flag, sigh). Because it seems that for the short jobs, we would like to just do "perf record java -XX:+WhatEver", followed by "perf report", without requiring user to invoke the diagnostic command while JVM is still running?

@plummercj
Copy link
Contributor

@plummercj plummercj commented Oct 20, 2020

/label add compiler

@openjdk openjdk bot added the compiler label Oct 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@plummercj
The compiler label was successfully added.

src/hotspot/share/code/codeCache.cpp Outdated Show resolved Hide resolved
src/hotspot/share/code/codeCache.cpp Outdated Show resolved Hide resolved
@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 21, 2020

Because it seems that for the short jobs, we would like to just do "perf record java -XX:+WhatEver", followed by "perf report", without requiring user to invoke the diagnostic command while JVM is still running?

Yes that sounds like a good idea. Add a (diagnostic?) option -XX:+WritePerfMapOnExit?

const char* method_name =
cb->is_compiled() ? cb->as_compiled_method()->method()->external_name()
: cb->name();
fs.print_cr(INTPTR_FORMAT " " INTPTR_FORMAT " %s",

This comment has been minimized.

@plummercj

plummercj Oct 21, 2020
Contributor

Indentation isn't right.

This comment has been minimized.

@nick-arm

nick-arm Oct 26, 2020
Author Member

Do you mean how the arguments are lined up on the continuation line below?

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Oct 22, 2020

Yes that sounds like a good idea. Add a (diagnostic?) option -XX:+WritePerfMapOnExit?

I think we should use this option carefully because nmethod might be unloaded. So we should use this with -XX:-UseCodeCacheFlushing.

BTW we can use Compiler.codelist dcmd for this purpose now. If you implement WritePerfMapOnExit, we should consider code cache flushing and should use Compiler.codelist in some case. I've published perfmap generator from Compiler.codelist https://github.com/YaSuenag/saperf

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Oct 22, 2020

\label remove compiler

Copy link
Member

@tstuefe tstuefe left a comment

Hi Nick,

this is a very useful idea! I missed this in the past.

Some remarks. I'll try to keep bikeshedding to a minimum since you already have enough input. Mostly ergonomics.

  1. Like Alexey, I would really wish for an print-at-exit switch. The common naming seems to be xxxAtExit (so not, OnExit). "PrintXxx" seems to be printing stuff out to tty, "DumpXxxx" for writing separate files (e.g. CDS map). So I would name it DumpPerfMapAtExit.

  2. Dumping to /tmp is unexpected for me, I would prefer if the default were dumping to the current directory. That seems to be the default for other files too (cds map, hs-err file etc).

  3. Not necessary but nice would be a an option to specify location of the dump file.

  4. I think it would be nice to have these switches always available, so real product switches. Which would require you to write up a small CSR but I still think it would make sense.

Cheers, Thomas

src/hotspot/share/code/codeCache.hpp Outdated Show resolved Hide resolved
src/hotspot/share/code/codeCache.cpp Outdated Show resolved Hide resolved
@plummercj
Copy link
Contributor

@plummercj plummercj commented Oct 22, 2020

/label add hotspot-compiler

@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2020

@plummercj
The hotspot-compiler label was successfully added.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 22, 2020

/label remove compiler

@openjdk openjdk bot removed the compiler label Oct 22, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2020

@nick-arm
The compiler label was successfully removed.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 22, 2020

1. Like Alexey, I would really wish for an print-at-exit switch. The common naming seems to be xxxAtExit (so not, OnExit). "PrintXxx" seems to be printing stuff out to tty, "DumpXxxx" for writing separate files (e.g. CDS map). So I would name it DumpPerfMapAtExit.

OK, makes sense.

2. Dumping to /tmp is unexpected for me, I would prefer if the default were dumping to the current directory. That seems to be the default for other files too (cds map, hs-err file etc).

3. Not necessary but nice would be a an option to specify location of the dump file.

The /tmp/perf-<pid>.map is hardcoded into perf though (see here), so I don't think it's useful for the user to be able to change the location.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 26, 2020

@nick-arm 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 8254723
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 27, 2020

/csr unneeded

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 27, 2020

I don't see any reason for this to be a product flag, rather than diagnostic.

OK sure, I've made it a diagnostic flag.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2020

@nick-arm only Reviewers can determine that a CSR is not needed.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 28, 2020

@nick-arm only Reviewers can determine that a CSR is not needed.

@dholmes-ora would you mind helping to remove the csr label? I don't have permission to do it.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Oct 28, 2020

/csr unneeded

@openjdk openjdk bot removed the csr label Oct 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2020

@dholmes-ora determined that a CSR request is not needed for this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2020

@nick-arm 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:

8254723: add diagnostic command to write Linux perf map file

Reviewed-by: ysuenaga, sspitsyn

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

  • 79a010f: 8255697: LogTargetHandle::print should check if log level is enabled
  • 98c91b6: 8233637: [TESTBUG] Swing ActionListenerCalledTwiceTest.java fails on macos
  • e97809d: 8233641: [TESTBUG] JMenuItem test bug4171437.java fails on macos
  • 69f5235: 8255596: Mutex safepoint checking options and flags should be scoped enums
  • d05df7c: 8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release
  • 64feeab: 8255603: Memory/Performance regression after JDK-8210985
  • 518ff51: 8233569: [TESTBUG] JTextComponent test bug6361367.java fails on macos
  • 4c4b8f4: 8196302: javax/swing/JFileChooser/8041694/bug8041694.java
  • f61ce32: 8255576: (fs) Files.isHidden() throws ArrayIndexOutOfBoundsException (unix)
  • fe7672b: 8196099: javax/swing/text/CSSBorder/6796710/bug6796710.java fails
  • ... and 100 more: https://git.openjdk.java.net/jdk/compare/b71b5b43754af423f5181ed671e82d083af00b87...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 label Oct 28, 2020
@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Oct 30, 2020

@YaSuenag could you re-review the latest changes?

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Oct 30, 2020

@YaSuenag could you re-review the latest changes?

Sure, the change looks good to me. However I don't understand why CSR is not needed. It introduces new dcmd for Linux.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Nov 2, 2020

Sure, the change looks good to me. However I don't understand why CSR is not needed. It introduces new dcmd for Linux.

I think because interfaces that are for diagnostic purposes don't require a CSR. See question 4 on the CSR FAQs:

https://wiki.openjdk.java.net/display/csr/CSR+FAQs

Copy link
Member

@YaSuenag YaSuenag left a comment

Ok, I reviewed this change. I guess you should close JDK-8254723.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Nov 3, 2020

/integrate

@openjdk openjdk bot closed this Nov 3, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm Since your change was applied there have been 136 commits pushed to the master branch:

  • f97ec35: 8255785: X11 libraries should not be required by configure for headless only
  • 184db64: 8255732: OpenJDK fails to build if $A is set to a value with spaces
  • c774741: 8255695: Some JVMTI calls in the jdwp debug agent are using FUNC_PTR instead of JVMTI_FUNC_PTR
  • bee864f: 8255766: Fix linux+arm64 build after 8254072
  • ceba2f8: 8255696: JDWP debug agent's canSuspendResumeThreadLists() should be removed
  • a250716: 8255694: memory leak in JDWP debug agent after calling JVMTI GetAllThreads
  • acb5f65: 8211958: Broken links in java.desktop files
  • bc6085b: 8255578: [JVMCI] be more careful about reflective reads of Class.componentType.
  • 05bcd67: 8255529: Remove unused methods from java.util.zip.ZipFile
  • d93e3a7: 8255760: Shenandoah: match constants style in ShenandoahMarkTask fallback
  • ... and 126 more: https://git.openjdk.java.net/jdk/compare/b71b5b43754af423f5181ed671e82d083af00b87...master

Your commit was automatically rebased without conflicts.

Pushed as commit 50357d1.

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

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

5 similar comments
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

openjdk-notifier bot referenced this pull request Nov 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

1 similar comment
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@nick-arm The command integrate can only be used in open pull requests.

@nick-arm nick-arm deleted the nick-arm:8254723 branch Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment