-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8365306: Provide OS Process Size and Libc statistic metrics to JFR #26756
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
base: master
Are you sure you want to change the base?
8365306: Provide OS Process Size and Libc statistic metrics to JFR #26756
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
4828a1b
to
b24c784
Compare
b24c784
to
39e282a
Compare
label /hotspot-jfr |
/label hotspot-jfr |
@stefank |
What is the problem with adding RSS metrics to the existing ResidentSetSize event? |
You mean adding the vsize, swap etc fields to ResidentSetSize? I thought about that, but then it would be weirdly misnamed. RSS has a very specific meaning. So we would have ResidentSetSize.vsize, ResidentSetSize.swap, ResidentSetSize.rss (?) I also thought about splitting them up and add one event per value, following the "ResidentSetSize" pattern. So, one event for "VirtualSize", one for "Swap" etc. Apart from not liking the fine granularity, having these fields grouped in one event has multiple advantages. Mainly, I can build myself graphs in JMC for all these fields in one graph and correlate all the values. It is also cheaper to obtain (just one parsing operation for /proc/meminfo, for instance). |
I was thinking something like this:
When it comes to non-rss metrics, there is a Swap event, but not sure it is appropriate? Regarding other metrics, perhaps they should go into other events, or perhaps new events should be created. I haven't had time to look into it.
Periodic events can be emitted with the same timestamp. That way they can be grouped together. This is what we do with NativeMemoryUsage and NativeMemoryTotal. These events will likely be around for a long time. We shouldn't design them just to match the workflow of one tool as it currently works. New functionality can be added in the future.
We should not sacrifice the design unless the overhead is significant. |
This provides the following new metrics:
ProcessSize
event (new, periodic)LibcStatistics
(new, periodic)-XX:TrimNativeHeapInterval
)NativeHeapTrim
(new, event-driven) (for both manual and automatic trims)JavaThreadStatistic
Notes:
we already have
ResidentSetSize
event, and the newProcessSize
event is a superset of that. I don't know how these cases are handled. I'd prefer to throw the old event out, but JMC has a hard-coded chart for RSS, so I kept it in unless someone tells me to remove it.Obviously, the libc events are very platform-specific. Still, I argue that these metrics are highly useful. We want people to use JFR and JMC; people include developers that are dealing with performance problems that require platform-specific knowledge to understand. See my comment in the JBS issue.
I provided implementations, as far as possible, to Linux, MacOS and Windows.
Testing:
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26756/head:pull/26756
$ git checkout pull/26756
Update a local copy of the PR:
$ git checkout pull/26756
$ git pull https://git.openjdk.org/jdk.git pull/26756/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26756
View PR using the GUI difftool:
$ git pr show -t 26756
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26756.diff
Using Webrev
Link to Webrev Comment