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
8176026: SA: Huge heap sizes cause a negative value to be displayed in the jhisto heap total #3087
8176026: SA: Huge heap sizes cause a negative value to be displayed in the jhisto heap total #3087
Conversation
👋 Welcome back ksakata! A progress list of the required criteria for merging this PR into |
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.
I see another cast-to-int related bug, and it's visible in your jhisto output (both before and after):
304: 1 16 jdk.internal.perf.Perf
305: 1 16 java.util.jar.JavaUtilJarAccessImpl
306: 254 2863336944 int[]
Total : 22803 -1430650672
The large int[]
array should be at the top of the list, not the bottom. The issue is in ObjectHistogramElement.java:
public int compare(ObjectHistogramElement other) {
return (int) (other.size - size);
}
So this will result in the returned value having the wrong sign if the difference between other.size
and size
is too large. In ObjectHistogram.java, just above the code you fixed, we have:
public List<ObjectHistogramElement> getElements() {
List<ObjectHistogramElement> list = new ArrayList<>();
list.addAll(map.values());
Collections.sort(list, new Comparator<>() {
public int compare(ObjectHistogramElement o1, ObjectHistogramElement o2) {
return o1.compare(o2);
}
});
return list;
}
So it looks like this is calling the buggy compare()
method. I think the fix is to have compare()
return -1, 0, or 1 depending on the long
value of (other.size - size)
, rather than just trying to return (other.size - size)
.
@plummercj That is right! I fixed that bug and pasted the latest output of jhisto to the body. Of course I re-ran the jtreg 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.
The copyright needs updating in ObjectHistogramElement.java. Otherwise the changes look good.
@jyukutyo 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 289 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. 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 (@plummercj, @kevinjwalls, @YaSuenag) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
I just updated the copyright. |
/integrate |
Could someone sponsor this pull request? It is ready. |
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. I will sponsor you.
/sponsor |
@YaSuenag @jyukutyo Since your change was applied there have been 289 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 39f0b27. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
When a heap is used more than about 2.1GB, clhsdb jhisto shows a negative number in the total field.
(Incidentally, the Sample is a program that only allocates many objects.)
Details
This is because in ObjectHistogram class the totalSize variable is int type.
The total size is the total of ObjectHistogramElement#getSize() and getSize() returns long. So I changed int to long in the ObjectHistogram class.
Additionally, I changed the type of the totalCount. This doesn't cause a bug, but ObjectHistogramElement#getCount() also returns long. So it doesn't need to treat it as int, I think. ObjectHistogramElement#compare() also do the same thing, so a class that is a huge size show the bottom of the list.
Tests
The jtreg test was successful.
I confirmed the output with the same program.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3087/head:pull/3087
$ git checkout pull/3087
Update a local copy of the PR:
$ git checkout pull/3087
$ git pull https://git.openjdk.java.net/jdk pull/3087/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3087
View PR using the GUI difftool:
$ git pr show -t 3087
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3087.diff