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

[Suchita Chaturvedi]

On Mon, 21 Jun 2021 15:23:06 GMT, Alex Macdonald <aptmac at openjdk.org> wrote:

>> Suchita Chaturvedi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed test failures
>
> 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)`.

Thanks for pointing it out. I have removed the same.

> 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.

Implemented this change. I tried the alternate as well but it started making the test case fail saying static inner class should be used. If I make that change there are few non static reference in TLABSummaryUI which will require changes at many places, so just dropped that idea.

> 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.

Agree. Removed.

> 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.

Yes. Customer was missing class page and summary page. Actually they both were present in JMC 5.5 and customer wanted it back. I have mentioned it in JIRA also.

> 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.

Agreed. Even I thought of Summary, but then since JMC 5.5 was using General so thought of replacing that literal to make it look similar to customer. They are strictly comparing both JMC 5.5 and JMC 8. But summary sounds better to I did this change.

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

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