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

8237354: Add option to jcmd to write a gzipped heap dump #209

Closed
wants to merge 4 commits into from
Closed

8237354: Add option to jcmd to write a gzipped heap dump #209

wants to merge 4 commits into from

Conversation

buddyliao
Copy link
Contributor

@buddyliao buddyliao commented Aug 9, 2021

Backport of the heap dump enhancement: Add option to jcmd to write a gzipped heap dump

Patch did not apply cleanly, I had to resolve/modify

  1. src/hotspot/share/services/heapDumper.cpp
    function "int dump(const char* path, bool overwrite = false)" has some different from the jdk-master, since patch 7cbb67a is backport before this one, so this patch can't backport clearly

  2. src/hotspot/share/services/heapDumperCompression.cpp
    MonitorLockerEx is named MonitorLocker in jdk-master, MonitorLockerEx.wait should have parameters, the constructor of MonitorLockerEx and MutexUnlockerEx also different from jdk-master

  3. src/hotspot/share/services/heapDumperCompression.hpp
    FileWriter is different from jdk-master since backport of 7cbb67a before this patch.

I have built it on linux x86_64 and run test-tier1


Progress

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

Issue

  • JDK-8237354: Add option to jcmd to write a gzipped heap dump

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/209/head:pull/209
$ git checkout pull/209

Update a local copy of the PR:
$ git checkout pull/209
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/209/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 209

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/209.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 9, 2021

👋 Welcome back buddyliao! 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 openjdk bot changed the title Backport 19be49714356f2f598924c9fefa6358679ab6dbe 8237354: Add option to jcmd to write a gzipped heap dump Aug 9, 2021
@openjdk
Copy link

openjdk bot commented Aug 9, 2021

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Aug 9, 2021
@mlbridge
Copy link

mlbridge bot commented Aug 9, 2021

Webrevs

@phohensee
Copy link
Member

Can you elaborate on what you had to change in the modified files?

@buddyliao
Copy link
Contributor Author

Can you elaborate on what you had to change in the modified files?

Hi Paul, thank you for your kindly review, I had elaborate on the changes at the beginning of this issue.

@phohensee
Copy link
Member

Thanks you for pointing that out. :) I was reading only the email.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

If MonitorLockerEx.wait() needs parameters, why does the call to wait() in heapDumperCompression.cpp at line 258 lack one? The two other uses pass Mutex::_no_safepoint_check_flag, which seems the correct thing to do given that all three constructors are passed Mutex::_no_safepoint_check_flag.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

I applied the patch and ran the serviceability/dcmd/gc tests with "make run-test". HeapDumpAllTest crashed due to the lack of the the line 258 change. Adding that cured that crash, but the new HeapDumpCompressedTest test failed due to lack of a backport of JDK-8233790. 8233790 guarantees that the "Unable to create" string for which the test checks will be in the output file. That backport looks useful to me, so I'd do it as a pre-req to this one.

@buddyliao
Copy link
Contributor Author

I applied the patch and ran the serviceability/dcmd/gc tests with "make run-test". HeapDumpAllTest crashed due to the lack of the the line 258 change. Adding that cured that crash, but the new HeapDumpCompressedTest test failed due to lack of a backport of JDK-8233790. 8233790 guarantees that the "Unable to create" string for which the test checks will be in the output file. That backport looks useful to me, so I'd do it as a pre-req to this one.

Hi Paul, thank you for pointing out the lack of line 258, I will fix it and update the patch. I know JDK-8233790 is a fix for the HeapDumpCompressedTest, I will backport it after this patch merge.

Bin Liao added 2 commits August 18, 2021 10:37
… backport-19be49714356f2f598924c9fefa6358679ab6dbe
@buddyliao
Copy link
Contributor Author

If MonitorLockerEx.wait() needs parameters, why does the call to wait() in heapDumperCompression.cpp at line 258 lack one? The two other uses pass Mutex::_no_safepoint_check_flag, which seems the correct thing to do given that all three constructors are passed Mutex::_no_safepoint_check_flag.

Thank you for pointing out this, I have fixed it.

@phohensee
Copy link
Member

I strongly recommend backporting JDK-8233790 first. The general update release rule is to backport dependencies first, then the dependent issue. It's easier to review that way. Also, I doubt the maintainers will approve a backport such as this one that breaks a test, even if the test is broken for only a short time. At a minimum, automated test systems will notice the failure and provoke work on the part of their owners.

@buddyliao
Copy link
Contributor Author

buddyliao commented Aug 19, 2021

I strongly recommend backporting JDK-8233790 first. The general update release rule is to backport dependencies first, then the dependent issue. It's easier to review that way. Also, I doubt the maintainers will approve a backport such as this one that breaks a test, even if the test is broken for only a short time. At a minimum, automated test systems will notice the failure and provoke work on the part of their owners.

Sounds reasonable, I will backport JDK-8233790 first. Thank you for your suggestion.

@buddyliao
Copy link
Contributor Author

I strongly recommend backporting JDK-8233790 first. The general update release rule is to backport dependencies first, then the dependent issue. It's easier to review that way. Also, I doubt the maintainers will approve a backport such as this one that breaks a test, even if the test is broken for only a short time. At a minimum, automated test systems will notice the failure and provoke work on the part of their owners.

Hi Paul, I have backported JDK-8233790 in #256, would you help me to review this patch first? Thank you.

@linzang
Copy link
Contributor

linzang commented Aug 20, 2021

I have helped to add "jdk11u-fix-request" label at https://bugs.openjdk.java.net/browse/JDK-8237354
Thanks!

Bin Liao added 2 commits August 26, 2021 10:42
…ithub.com:buddyliao/jdk11u-dev into backport-19be49714356f2f598924c9fefa6358679ab6dbe
@openjdk
Copy link

openjdk bot commented Aug 26, 2021

⚠️ @buddyliao This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@buddyliao
Copy link
Contributor Author

I strongly recommend backporting JDK-8233790 first. The general update release rule is to backport dependencies first, then the dependent issue. It's easier to review that way. Also, I doubt the maintainers will approve a backport such as this one that breaks a test, even if the test is broken for only a short time. At a minimum, automated test systems will notice the failure and provoke work on the part of their owners.

Hi @phohensee , JDK-8233790 has been backported and merged, I have rebased this patch and test tier1. Would you please continue reviewing this backport? Thank you.

@schmelter-sap
Copy link
Contributor

Note that the ParallelScavengeHeap in JDK11 doesn't support the workers() method, since it doesn't uses a work gang. This means that the compression will not be done in parallel in this GC. This is probably not show stopper, but a regression from the behavior found in current JDKs.

Copy link
Contributor

@schmelter-sap schmelter-sap left a comment

Choose a reason for hiding this comment

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

The change looks good to me.

@openjdk
Copy link

openjdk bot commented Sep 12, 2021

@buddyliao This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8237354: Add option to jcmd to write a gzipped heap dump

Reviewed-by: rschmelter, clanger

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

  • 126f1a2: 8225083: Remove Google certificate that is expiring in December 2021
  • 90ecf27: 8213263: fix legal headers in test/langtools
  • dee030c: 8210353: Move java/util/Arrays/TimSortStackSize2.java back to tier1
  • 5a5d246: 8195703: BasicJDWPConnectionTest.java: 'App exited unexpectedly with 2'
  • 180bc52: 8272966: test/jdk/java/awt/Robot/FlushCurrentEvent.java fails by timeout
  • bfb52f2: 8273547: [11u] [JVMCI] Partial module-info.java backport of JDK-8223332
  • 1e6682a: 8247403: JShell: No custom input (e.g. from GUI) possible with JavaShellToolBuilder
  • 4ea2edd: 8260690: JConsole User Guide Link from the Help menu is not accessible by keyboard
  • a39375e: 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font
  • a013fe2: 8210395: Add doc to SecurityTools.java
  • ... and 27 more: https://git.openjdk.java.net/jdk11u-dev/compare/5d1b9bbe8328305d41d7756a55a639c48af857c7...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@phohensee, @RealCLanger) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2021
@buddyliao
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 13, 2021
@openjdk
Copy link

openjdk bot commented Sep 13, 2021

@buddyliao
Your change (at version 94f8271) is now ready to be sponsored by a Committer.

@RealCLanger
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 13, 2021

Going to push as commit 1868c15.
Since your change was applied there have been 37 commits pushed to the master branch:

  • 126f1a2: 8225083: Remove Google certificate that is expiring in December 2021
  • 90ecf27: 8213263: fix legal headers in test/langtools
  • dee030c: 8210353: Move java/util/Arrays/TimSortStackSize2.java back to tier1
  • 5a5d246: 8195703: BasicJDWPConnectionTest.java: 'App exited unexpectedly with 2'
  • 180bc52: 8272966: test/jdk/java/awt/Robot/FlushCurrentEvent.java fails by timeout
  • bfb52f2: 8273547: [11u] [JVMCI] Partial module-info.java backport of JDK-8223332
  • 1e6682a: 8247403: JShell: No custom input (e.g. from GUI) possible with JavaShellToolBuilder
  • 4ea2edd: 8260690: JConsole User Guide Link from the Help menu is not accessible by keyboard
  • a39375e: 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font
  • a013fe2: 8210395: Add doc to SecurityTools.java
  • ... and 27 more: https://git.openjdk.java.net/jdk11u-dev/compare/5d1b9bbe8328305d41d7756a55a639c48af857c7...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 13, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 13, 2021
@openjdk
Copy link

openjdk bot commented Sep 13, 2021

@RealCLanger @buddyliao Pushed as commit 1868c15.

💡 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
backport integrated Pull request has been integrated
5 participants