-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8319650: Improve heap dump performance with class metadata caching #16545
Conversation
👋 Welcome back shade! A progress list of the required criteria for merging this PR into |
8f0b8c9
to
a19501f
Compare
@shipilev Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good. I only have a questions related to resource management (see inline).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@shipilev 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:
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 14 new commits pushed to the
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 |
What is the proportion of RSS growth caused by class caching during heapdump? As we create class cache entry(num_of_fields*8+ len_of_sig + u4_instance_size + u4_entries) per instance klass |
Good question! On simple Hello World like scenarios, NMT shows about 80 bytes of additional But then nothing really requires us to cache all the classes all the time, or track the class popularity. We can just clear the entire table every so often, and let it regenerate with the most frequent classes again and again. If we cache only the 256 most recently popular classes, then the final number from the extreme example above gets to the modest 20M of additional memory. In practice, with normal thread counts, I would expect the overhead to be in double-digit KBs. Implemented in new commit. I checked with |
A bit more realistic test,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I have a couple of minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for taking extra memory consumption into consideration.
If there are no other comments, I would like to integrate it soon. Thank you all! |
/integrate |
Going to push as commit 03db828.
Your commit was automatically rebased without conflicts. |
See the rationale and some profiles in the bug.
This PR implements simple caching for object instance dumping, which is overwhelmingly hottest path in heap dumping code. This allows to put the cache straight in
HeapObjectDumper
, which is effectively thread local. A simple hash table would then work to cache the class metadata.Given a simple workload from the bug, these are the motivational improvements and historical perspective:
This amounts to 5x improvement, 1.2 GB/sec dump rate (somewhat I/O limited), and the best heap dump performance across all JDK releases. I am planning to backport this patch where possible: at least to JDK 21u, but hopefully to JDK 17u as well.
There are other simple improvements, but this one is the top bottleneck.
Additional testing:
serviceability/
(contains heap dump tests)runtime/ErrorHandling
(contains heap dump on failure tests)gc/epsilon
(contains heap dump on failure tests)sun/tools/jhsdb
(contains heap dump tests)Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16545/head:pull/16545
$ git checkout pull/16545
Update a local copy of the PR:
$ git checkout pull/16545
$ git pull https://git.openjdk.org/jdk.git pull/16545/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16545
View PR using the GUI difftool:
$ git pr show -t 16545
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16545.diff
Webrev
Link to Webrev Comment