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

6657: Add allocation pressure column to Memory and TLAB views #21

Closed
wants to merge 19 commits into from

Conversation

@jiekang
Copy link
Contributor

jiekang commented Dec 18, 2019

This patch addresses https://bugs.openjdk.java.net/browse/JMC-6657, which itself is a clone of https://bugs.openjdk.java.net/browse/JMC-5923 that targets the Memory and TLAB pages, rather than the stacktrace view.

The ItemHistogram and related classes now have support for Percentage columns that divides the aggregate value for items in a row against the aggregate value for all items in the model.

This is used in the Memory page to show Total Allocation as a Percentage. The TLAB page is modified to be tabbed and contains two ItemHistograms, one for classifying against Threads (previously existed) and one against Top Methods (new). Both also have two new columns: allocations inside and outside TLABs, as a percentage.

tlab-page
TLABs by Top Methods, showcasing sorting by the new percentage column. Together with the stacktrace view, I believe these changes make it easier to see relevant areas of allocation pressure.


Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-6657: Add allocation pressure column to Memory and TLAB views

Reviewers

  • Marcus Hirt (hirt - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jmc pull/21/head:pull/21
$ git checkout pull/21

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 18, 2019

👋 Welcome back jkang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Dec 18, 2019
@mlbridge
Copy link

mlbridge bot commented Dec 18, 2019

@thegreystone
Copy link
Collaborator

thegreystone commented Dec 19, 2019

Hi Jie,

Thank you for looking at this! Here's some feedback before reviewing the code:

Memory tab:

  1. "Total Allocation Percentage" is a bit long. Can this be shortened? The description will be there is someone hovers over the header, for someone wanting more info. Maybe "Total Allocation (%)"?
  2. Should there perhaps be a backdrop bar in that column, for the percentage?

TLAB allocations:

  1. Same here, perhaps shorten the column width by having "Total Allocation (%)".
  2. Backdrop bar in that column too?
  3. Instead of spelling out Average, perhaps go with Avg? Instead of Allocation go with Alloc? Normally I'm against these kinds of shorthands, but this is a specialized tab, and some of these columns get really wide since the titles are so long. Much wider than their content, leading to waste of screen estate.
@egahlin
Copy link

egahlin commented Dec 19, 2019

I think JMC 5.5 had good pick of default columns and names.
https://www.javacodegeeks.com/wp-content/uploads/2015/03/jfr-allocations.png

@mlbridge
Copy link

mlbridge bot commented Dec 19, 2019

Mailing list message from Marcus Hirt on jmc-dev:

In JMC 5.5 there was a split between the event types (and successive
breakdowns in tabs and tabs in tabs), in later versions there is an
attempt to give an overview of both inside and outside of TLAB
allocations, and their relation, in one table and a graph. There are
advantages to both. If anything, I miss having the means to switch the
aggregation between thread and allocated class name and the pressure
column.

On Thu, Dec 19, 2019 at 3:55 PM Erik Gahlin <egahlin at openjdk.java.net> wrote:

On Thu, 19 Dec 2019 14:43:30 GMT, Marcus Hirt <hirt at openjdk.org> wrote:

This patch addresses https://bugs.openjdk.java.net/browse/JMC-6657, which itself is a clone of https://bugs.openjdk.java.net/browse/JMC-5923 that targets the Memory and TLAB pages, rather than the stacktrace view.

The ItemHistogram and related classes now have support for Percentage columns that divides the aggregate value for items in a row against the aggregate value for all items in the model.

This is used in the Memory page to show Total Allocation as a Percentage. The TLAB page is modified to be tabbed and contains two ItemHistograms, one for classifying against Threads (previously existed) and one against Top Methods (new). Both also have two new columns: allocations inside and outside TLABs, as a percentage.

![tlab-page](https://user-images.githubusercontent.com/5430520/71093691-b2094c00-2177-11ea-9965-f27948964cd3.png)
TLABs by Top Methods, showcasing sorting by the new percentage column. Together with the stacktrace view, I believe these changes make it easier to see relevant areas of allocation pressure.

Hi Jie,

Thank you for looking at this! Here's some feedback before reviewing the code:

Memory tab:
1. "Total Allocation Percentage" is a bit long. Can this be shortened? The description will be there is someone hovers over the header, for someone wanting more info. Maybe "Total Allocation (%)"?
2. Should there perhaps be a backdrop bar in that column, for the percentage?

TLAB allocations:
1. Same here, perhaps shorten the column width by having "Total Allocation (%)".
2. Backdrop bar in that column too?
3. Instead of spelling out Average, perhaps go with Avg? Instead of Allocation go with Alloc? Normally I'm against these kinds of shorthands, but this is a specialized tab, and some of these columns get really wide since the titles are so long. Much wider than their content, leading to waste of screen estate.

I think JMC 5.5 had good pick of default columns and names.
https://www.javacodegeeks.com/wp-content/uploads/2015/03/jfr-allocations.png

-------------

PR: https://git.openjdk.java.net/jmc/pull/21

@egahlin
Copy link

egahlin commented Dec 19, 2019

I think we can skip any mention of TLABs, at least if the purpose of the view is to find where the top allocation sites are.

I think Class | Sample Count | Pressure will be sufficient for 99% of all users.

There could be a seperate tab for those that want to tune TLABs, or hidden columns that can be made visible for advanced users. In the future, we will probably add an ObjectAllocation event that can be on by default that will not leak implementation details such TLAB information.

@jiekang
Copy link
Contributor Author

jiekang commented Dec 19, 2019

@thegreystone

Thanks for taking a look. I have shortened the names and added translations. I am also looking at moving the percentage aggregators (e.g. JdkAggregators.ALLOC_INSIDE_TLAB_SUM_PERCENTAGE) out of core, probably into the TlabPage related code, as well as adding something there to make it a cleaner solution.

It's a bit ugly since the aggregator does not compute a percentage itself. I've been toying with writing some helper classes or functions to work over multiple aggregators and item collections but I haven't come up with anything clean and it's outside the scope of this issue, imo.

@egahlin

Thank you for sharing the screenshot. I like the setup shown there as well. I will see if I can follow-up the changes here with some more adjustments to the Memory/Tlab page UI in JDK Mission Control.

Having the Stacktrace View be generic and available for all pages has made things a bit more complicated to mimic what you have shown. It may be cool to have some kind of extension functionality such that when a page is visited, the page can augment this generic view with additional columns (e.g. to show allocation pressure by method while on the memory page).

Regarding removal of reference to TLAB and having more advanced options via initially hidden features (e.g. columns), I think what you wrote makes sense. It may be that what you described is close to what is in JDK Mission Control. The Memory page shows events by class and includes the percentage column for total allocations. It has initially hidden columns for in/out of TLAB related aggregations. The TLAB page on the other hand, (screenshot in initial description) shows these TLAB events and now by thread or by top method. I think the organization of the UI can be improved and I'm going to think about and potentially propose more changes after this. (I should have also included a screenshot of the Memory page...)

@thegreystone
Copy link
Collaborator

thegreystone commented Dec 20, 2019

Yep, it's already split up in a general "Memory" page and a TLAB allocations page (which is located under JVM Internals), and which is already grouped by class. The only missing part would be the "Pressure", which is basically Total Allocation (%), added by Jie's PR.

It may be cool to have some kind of extension functionality such that when a page is visited, the page can augment this generic view with additional columns (e.g. to show allocation pressure by method while on the memory page).

Agreed. I believe this is discussed in another already existing issue. We can start a thread on the dev-list to discuss this in more detail.

aptmac added a commit to aptmac/jmc that referenced this pull request Jan 2, 2020
@thegreystone
Copy link
Collaborator

thegreystone commented Jan 14, 2020

Note that copyrights need to be updated.

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 20, 2020

Hi Jie!

How about hiding the count column, or even removing it? I believe this is the event count for both of the TLAB events - it does not correspond to the number of actual objects allocated. We could perhaps estimate the number of allocated objects (using the size of the allocated object inside tlab and the tlab size) and add that as a new column, with a proper description, but the total event count doesn't really make sense. Now that we have the percentages, we may want to instead have the backdrop bars on them.

Also, how about hiding the average columns by default? There is a bit of information overload right now, and I think the sums may be what people will usually want. They can show the other columns if they need them.

@jiekang jiekang force-pushed the jiekang:jmc-5923 branch from 867c2bb to b5ddfdc Jan 20, 2020
@jiekang
Copy link
Contributor Author

jiekang commented Jan 22, 2020

Hi Jie!

How about hiding the count column, or even removing it? I believe this is the event count for both of the TLAB events - it does not correspond to the number of actual objects allocated. We could perhaps estimate the number of allocated objects (using the size of the allocated object inside tlab and the tlab size) and add that as a new column, with a proper description, but the total event count doesn't really make sense. Now that we have the percentages, we may want to instead have the backdrop bars on them.

Also, how about hiding the average columns by default? There is a bit of information overload right now, and I think the sums may be what people will usually want. They can show the other columns if they need them.

I agree with your suggestions. I've removed the count column and hidden the average columns by default. I am currently working on the backdrop bars. It's a bit more complicated than I had initially hoped.

I think an estimated count of number of objects allocated could be useful. I can also try to add this here, but I think it's extending a little more past the issue contents. Is it acceptable if I open another issue in JIRA to address this?

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 22, 2020

Is it acceptable if I open another issue in JIRA to address this?

Certainly!

@jiekang
Copy link
Contributor Author

jiekang commented Jan 22, 2020

Is it acceptable if I open another issue in JIRA to address this?

Certainly!

Created @ https://bugs.openjdk.java.net/browse/JMC-6678

@jiekang jiekang force-pushed the jiekang:jmc-5923 branch from c819e9d to e20943e Feb 7, 2020
@jiekang
Copy link
Contributor Author

jiekang commented Feb 7, 2020

@thegreystone Hey Marcus, the latest commit adds the backdrop bars, and the entire chain of commits in the PR is rebased on top of master commit 6dd19dd

I'm still trying to think of ways to clean up the code for backdrop bars: looking at the unit infrastructure to see how to extract the column value without needing to divide by 100, if possible, and just the addition of the code straight into the addPercentageColumn code is a bit crude and not so extensible. It works as far as I can tell, at the least.

@thegreystone
Copy link
Collaborator

thegreystone commented Feb 19, 2020

@jiekang - Hi Jie! Did you want to spend some more time looking at ways to clean up the code for the backdrop bars before integrating this?

@jiekang jiekang force-pushed the jiekang:jmc-5923 branch from e20943e to 2ca1510 Feb 19, 2020
@jiekang
Copy link
Contributor Author

jiekang commented Feb 19, 2020

@thegreystone Hey; sorry a roller-coaster month of sorts. I just made it a little more readable (I hope) and rebased onto latest. I don't have plans to do more for the near future. I will still address any requests with vigor though! I do want to see it through.

@thegreystone
Copy link
Collaborator

thegreystone commented Mar 2, 2020

image

I've been thinking of how we can make the naming clearer. What do you think about:

  • Avg Alloc in TLABs
  • Avg Alloc Outside TLABs
  • Total Alloc in TLABs
  • Total Alloc Outside TLABs

Reasoning:

  • Just saying Avg. TLAB Alloc can be confusing - it's not the TLAB allocation but rather the estimated size of allocations in the TLAB, on average. Also rhymes better with Avg Alloc Outside TLABs. We do explain that it is an estimate in the tooltip, so perhaps we can skip it in the name. We should either have it in both of the inside TLAB columns, or in none of them.
  • I think we can probably drop the contraction periods, but if someone disagrees I don't mind.
@thegreystone
Copy link
Collaborator

thegreystone commented Mar 3, 2020

Noted that I hadn't cleared my settings. This is how things look right now:
image

So, it becomes:

  • Alloc in TLABs
  • Alloc in TLABs (%)
  • Alloc Outside TLABs
  • Alloc Outside TLABs (%)

With the hidden ones e.g. being:

  • Avg Alloc in TLABs
  • Avg Alloc Outside TLABs

What do you think?

@jiekang jiekang force-pushed the jiekang:jmc-5923 branch from 7faa85a to d74ffea Mar 3, 2020
@jiekang
Copy link
Contributor Author

jiekang commented Mar 3, 2020

I agree with the naming suggestions and have updated the PR with a commit to match them, rebasing onto latest upstream as well.

@thegreystone
Copy link
Collaborator

thegreystone commented Mar 3, 2020

image

A little bit more default room for the Outside TLAB columns and we're good to go! :)

@jiekang
Copy link
Contributor Author

jiekang commented Mar 3, 2020

I've updated the default widths. When testing I made sure to clean the jmc cache (on Linux @ ~/.jmc) so the defaults would be seen. The intent is that the columns should have enough space for their title.

Copy link
Collaborator

thegreystone left a comment

Good to go! :) Thanks Jie!

@openjdk
Copy link

openjdk bot commented Mar 3, 2020

@jiekang This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

6657: Add allocation pressure column to Memory and TLAB views

Reviewed-by: hirt
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 49da5546cb5b0165a7e6fdda99d27149c5ae790b.

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 (@thegreystone) 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 label Mar 3, 2020
@jiekang
Copy link
Contributor Author

jiekang commented Mar 4, 2020

/integrate

@openjdk
Copy link

openjdk bot commented Mar 4, 2020

@jiekang
Your change (at version bc99ef6) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Mar 4, 2020
@thegreystone
Copy link
Collaborator

thegreystone commented Mar 4, 2020

/sponsor

@openjdk openjdk bot closed this Mar 4, 2020
@openjdk openjdk bot added integrated and removed sponsor ready labels Mar 4, 2020
@openjdk
Copy link

openjdk bot commented Mar 4, 2020

@thegreystone @jiekang The following commits have been pushed to master since your change was applied:

  • 49da554: 6712: Make agent retransform classes when loaded dynamically
  • 0915574: 6700: Make Flame Graph compile on Photon

Your commit was automatically rebased without conflicts.

Pushed as commit 28d203a.

@openjdk openjdk bot removed the rfr label Mar 4, 2020
@thegreystone
Copy link
Collaborator

thegreystone commented Mar 4, 2020

Thanks Jie! :)

@mlbridge
Copy link

mlbridge bot commented Mar 9, 2020

Mailing list message from Marcus Hirt on jmc-dev:

Changeset: 28d203a
Author: Jie Kang <jkang at openjdk.org>
Committer: Marcus Hirt <hirt at openjdk.org>
Date: 2020-03-04 16:19:21 +0000
URL: https://git.openjdk.java.net/jmc/commit/28d203af

6657: Add allocation pressure column to Memory and TLAB views

Reviewed-by: hirt

! application/l10n/org.openjdk.jmc.flightrecorder.ui.ja/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages_ja.properties
! application/l10n/org.openjdk.jmc.flightrecorder.ui.zh_CN/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages_zh_CN.properties
! application/org.openjdk.jmc.flightrecorder.ui/defaultPages.xml
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/AggregationGrid.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/ItemHistogram.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/messages/internal/Messages.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/ChartAndTableUI.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/HeapPage.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/JavaApplicationPage.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/ThreadsPage.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/TlabPage.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties
! core/org.openjdk.jmc.flightrecorder/src/main/resources/org/openjdk/jmc/flightrecorder/jdk/messages/internal/messages.properties

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.