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-8260282: Add option to compress heap dumps created by -XX:+HeapDumpOnOutOfMemoryError #2222

Closed
wants to merge 8 commits into from

Conversation

@schmelter-sap
Copy link
Contributor

@schmelter-sap schmelter-sap commented Jan 25, 2021

This change adds the optiont to created a gzipped heap dump by -XX:+HeapDumpOnOutOfMemoryError.

-XX:HeapDumpGzipLevel= sets the compression level. 0 (the default) means no gzipping of the dump. Otherwise the level has to be between 1 and 10.


Progress

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

Issue

  • JDK-8260282: Add option to compress heap dumps created by -XX:+HeapDumpOnOutOfMemoryError

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 25, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 25, 2021

@schmelter-sap 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.

Loading

@openjdk openjdk bot added the rfr label Jan 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 25, 2021

Webrevs

Loading

"When HeapDumpOnOutOfMemoryError is on, the gzip compression " \
"level of the dump file. 0 (the default) disables gzip " \
"compression. Otherwise the level must be between 1 and 10.") \
range(0, 10) \
Copy link
Contributor

@plummercj plummercj Jan 25, 2021

Choose a reason for hiding this comment

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

Isn't the range 0-9? Although HeapDumper::dump() imposes no upper limit (it seems it should), the HeapDumpDCMD sets the upper limit to 9. See HeapDumpDCmd::execute().

Loading

Copy link
Contributor Author

@schmelter-sap schmelter-sap Jan 25, 2021

Choose a reason for hiding this comment

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

You're right. 9 is the maximum compression level supported by zlib. I will update the PR.

Loading

Copy link
Contributor

@plummercj plummercj left a comment

You need to run the test with a limited sized heap. Otherwise it may impact other tests, and would need to be moved into the resourcehogs directory.

Loading

@schmelter-sap
Copy link
Contributor Author

@schmelter-sap schmelter-sap commented Jan 26, 2021

You need to run the test with a limited sized heap. Otherwise it may impact other tests, and would need to be moved into the resourcehogs directory.

I've added a -Xmx128M. This should probably also done to TestHeapDumpOnOutOfMemoryError, which uses the same code.

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Jan 26, 2021

I've added a -Xmx128M. This should probably also done to TestHeapDumpOnOutOfMemoryError, which uses the same code.

Yes, it should. Probably it hasn't caused failures because my recollection is that it usually takes 2 or more such heap exhaustion tests running in parallel to result in unexpected failures in our testing, but it really depends on the test host and available memory. Also, the metaspace version of the test does use -XX:MaxMetaspaceSize=16m, so it doesn't use that much memory.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 26, 2021

@schmelter-sap 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:

8260282: Add option to compress heap dumps created by -XX:+HeapDumpOnOutOfMemoryError

Reviewed-by: cjplummer, stuefe

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

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.

Loading

@openjdk openjdk bot added the ready label Jan 26, 2021
@plummercj
Copy link
Contributor

@plummercj plummercj commented Jan 26, 2021

I've added a -Xmx128M. This should probably also done to TestHeapDumpOnOutOfMemoryError, which uses the same code.

Yes, it should. Probably it hasn't caused failures because my recollection is that it usually takes 2 or more such heap exhaustion tests running in parallel to result in unexpected failures in our testing, but it really depends on the test host and available memory. Also, the metaspace version of the test does use -XX:MaxMetaspaceSize=16m, so it doesn't use that much memory.

Looks like this was just addressed today by JDK-8252545.

Loading

Copy link
Member

@tstuefe tstuefe left a comment

Looks good. Nitpickings below, but I leave it up to you which ones you follow up to. Otherwise fine.

..Thomas

Loading

* @test
* @summary Test verifies that -XX:HeapDumpGzipLevel=0 works
* @library /test/lib
* @run driver/timeout=240 TestGZippedHeapDumpOnOutOfMemoryError run 0
Copy link
Member

@tstuefe tstuefe Jan 28, 2021

Choose a reason for hiding this comment

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

Is the timeout needed? Should the oome not be pretty much instantaneous?

Loading

Copy link
Contributor Author

@schmelter-sap schmelter-sap Jan 28, 2021

Choose a reason for hiding this comment

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

The timeout is not for the VM which creates the OOM, but the VM which starts that VM and then parses the created hprof file. I took the timeout from the TestHeapDumpOnOutOfMemoryError test, ,which seemed to run fine with this value.

Loading

* @summary Test verifies that -XX:HeapDumpGzipLevel=1 works
* @library /test/lib
* @run driver/timeout=240 TestGZippedHeapDumpOnOutOfMemoryError run 1
*/
Copy link
Member

@tstuefe tstuefe Jan 28, 2021

Choose a reason for hiding this comment

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

If the tests are fast enough, I would add at least another one at the end of the valid gzip range.

Loading

Copy link
Contributor Author

@schmelter-sap schmelter-sap Jan 28, 2021

Choose a reason for hiding this comment

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

I don't think that this is needed, since these test should not test the gzipping (there are already tests for that).

Loading

String heapdumpFilename = "java_pid" + proc.pid() + ".hprof" + (level > 0 ? ".gz" : "");
OutputAnalyzer output = new OutputAnalyzer(proc);
output.stdoutShouldNotBeEmpty();
output.shouldContain("Dumping heap to " + heapdumpFilename);
Copy link
Member

@tstuefe tstuefe Jan 28, 2021

Choose a reason for hiding this comment

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

Could this be a full path? In case printing on the VM side is ever changed to print the full path, I'd make this more lenient. Maybe two separate contains expressions or a shouldMatch with ".*" int he middle.

Loading

Copy link
Contributor Author

@schmelter-sap schmelter-sap Jan 28, 2021

Choose a reason for hiding this comment

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

I think that if the VM output changes, it is OK to adjust the test. Making it more lenient could also make it less accurate.

Loading

@schmelter-sap
Copy link
Contributor Author

@schmelter-sap schmelter-sap commented Jan 29, 2021

/integrate

Loading

@openjdk openjdk bot closed this Jan 29, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@schmelter-sap Since your change was applied there have been 68 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit d2b0ea1.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants