RFR: 7042: Bring back summary screen for Statistics on Object Allocation outside TLAB. [v2]

[Alex Macdonald]

On Sun, 20 Jun 2021 16:52:50 GMT, Suchita Chaturvedi <schaturvedi at openjdk.org> wrote:

>> This PR addresses the enhancement of adding General screen (providing summary) and By Class (table and graph) for allocations inside and outside TLAB.
>> 
>> The changes include the enhancement "7043: Bring back summary screen for Statistics on Object Allocation inside TLAB" as well.
>> 
>> Note: This is bring back feature i.e. which was present in JMC 5.5 but was missing in recent releases.
>> 
>> Please review the change and provide your valuable comments.
>
> Suchita Chaturvedi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed test failures

Overall the page looks good, I've left a couple of review comments inline.

For anyone that doesn't feel like building this pr, here's a screenshot of what it looks like:
Tab group:
![2021-06-21-121513_331x35_scrot](https://user-images.githubusercontent.com/10425301/122794732-914d1580-d28a-11eb-9ca7-2364a7c7b8cf.png)

Class Page:
![2021-06-21-121713_1494x736_scrot](https://user-images.githubusercontent.com/10425301/122794772-9e6a0480-d28a-11eb-94db-5d12bb7ba26e.png)

Summary Page:
![2021-06-21-121729_1490x739_scrot](https://user-images.githubusercontent.com/10425301/122794812-a88c0300-d28a-11eb-8e16-0994c0c63a7c.png)

application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java line 1171:

> 1169: 	}
> 1170: 
> 1171: 	public static void addTabItem(CTabFolder tabFolder, Object data, String name) {

This isn't used anywhere and can be removed; your new classCT and summaryCT in TlabPage make use of the existing `addTabItem(CTabFolder tabFolder, Control section, String name)`.

application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/TlabPage.java line 152:

> 150: 		}
> 151: 
> 152: 		public IPageUI display(Composite parent, FormToolkit toolkit, IPageContainer pageContainer, IState state) {

This could have been a `void` method. There isn't anything being performed on a return value here so there's no need to return null.

Alternatively all of this could be placed in the `TlabSummaryUI` constructor instead.

application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/TlabPage.java line 155:

> 153: 			form = DataPageToolkit.createForm(parent, toolkit, getName(), getIcon());
> 154: 			SashForm container = new SashForm(form.getBody(), SWT.HORIZONTAL);
> 155: 			container.setSashWidth(5);

I don't think the sashform doesn't need it's width to be set in this case.

application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/TlabPage.java line 227:

> 225: 			threadsCT.saveTo(state);
> 226: 			methodsCT.saveTo(state);
> 227: 			classCT.saveTo(state);

This page looks fine to me as well, but it's a bit of a stealth update as there wasn't mention in the bug report or issue name regarding adding a class page.

application/org.openjdk.jmc.flightrecorder.ui/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties line 526:

> 524: TlabPage_METHODS_TAB_NAME=By Top Methods
> 525: TlabPage_CLASS_TAB_NAME=By Class
> 526: TlabPage_SUMMARY_TAB_NAME=General

Minor nit, but I think "Summary" better describes the page than "General", and also follows the naming convention used in the code.

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

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