[Rev 02] RFR: 6810: Create treemap viewer for JOverflow
Alex Macdonald
aptmac at openjdk.java.net
Mon Jun 15 19:28:55 UTC 2020
On Tue, 9 Jun 2020 15:16:27 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:
>> This PR implements [JMC-6810: Create treemap viewer for JOverflow](https://bugs.openjdk.java.net/browse/JMC-6810). It
>> adds a treemap displaying memory usage by classes. It makes easier to spot what is consuming most of the memory. Like
>> JOverflow instance viewer, the treemap viewer utilizes the filtering feature provided by JOverflow.
>> 
>> The treemap is not included in the building by default, therefore you'll need to manually enable the
>> `org.openjdk.jmc.feature.joverflow.ext.treemap` feature explicitly. Once enabled, the treemap view part can be opened
>> in *Window* -> *Show View* -> *Other* -> *JOverflow* -> *JOverflow Treemap*.
>> ---
>>
>> The treemap view part consists of three components: the toolbar actions, the breadcrumb, and the treemap. All three
>> components are automatically updated when the JOverflow filter changes, or when a new JOverflow editor page becomes
>> active.
>>
>> ### Toolbar actions:
>>
>> 
>>
>> Buttons are enabled and disabled automatically according to the state of the treemap.
>>
>> - Zoom-in button: zoom-in to the selected node of the treemap
>> - Zoom-out button: zoom-out to the parent node of the currently expanded node of the treemap
>> - Zoom-off button: display the root node (ie. "[ROOT]" node)
>>
>> ### Breadcrumb
>>
>> 
>>
>> The breadcrumb indicates the location of the currently expanded treemap node relative to the root. Breadcrumb entries
>> are updated upon treemap expands/collapse. User interactions on the breadcrumb also updates the treemap.
>> - Left mouse click: navigate to desired zooming level.
>>
>> ### Treemap
>>
>> 
>>
>> The treemap implements the squarified treemap algorithm. Memory usage is aggregated by class and package names. The
>> larger the area, the more memory consumed by this class/package. Treemap nodes on different levels are labelled with
>> different background colours. User interactions on the treemap also updates the breadcrumb.
>> - Left mouse click: select and highlight the clicked node, allowing zooming-in with the toolbar button.
>> - Middle mouse click: display the root node (ie. "[ROOT]" node)
>> - Right mouse click: zoom-out to the parent node of the currently expanded node
>> - Left mouse double click: zoom-in to the clicked node (or to its parent node if the clicked node is a leaf)
>> - Mouse hover: displays the tooltip with the full qualified package/class name and the memory usage in a human-readable
>> format.
>>
>> ---
>>
>> Let me know what you think about the design and functionality. :)
>
> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>
> fixed failing to create the part's controls due to NPE
Hi Arvin,
I've given this a look through, and it looks good, just have some minor nits that I've added as review comments.
application/org.openjdk.jmc.feature.joverflow.ext.treemap/feature.properties line 37:
> 36: copyright=Copyright \u00A9 2020, Oracle and/or its affiliates. All rights reserved.
> 37: description=This feature adds a treemap view to JOverflow visualizing memory usage by packages/classes.
> 38: descriptionUrl=http://www.oracle.com/missioncontrol
Perhaps there should be a 'for' in between JOverflow and visualizing:
"This feature adds a treemap view to JOverflow _for_ visualizing memory usage by packages/classes."
application/org.openjdk.jmc.joverflow.ext.treemap/src/main/java/org/openjdk/jmc/joverflow/ext/treemap/TreemapAction.java
line 41:
> 40:
> 41: /* package-private */ class TreemapAction extends Action {
> 42: private final TreemapActionType actionType;
I guess this comment kind of applies across the whole patch; I'm not against having a package private comment (it's
kind of nice for the methods at least IMO), however if kept I'd prefer if they followed a similar standard.
For example, here it's placed before the class, however elsewhere like `Treemap.checkNull()` it's on the previous line.
application/org.openjdk.jmc.joverflow.ext.treemap/src/main/java/org/openjdk/jmc/joverflow/ext/treemap/TreemapAction.java
line 67:
> 66: enum TreemapActionType {
> 67: ZOOM_IN(Messages.TreemapAction_ZOOM_IN_DESCRIPTION, IAction.AS_PUSH_BUTTON, CoreImages.ZOOM_IN), //
> 68: ZOOM_OUT(Messages.TreemapAction_ZOOM_OUT_DESCRIPTION, IAction.AS_PUSH_BUTTON, CoreImages.ZOOM_OUT), //
More of a note, but I'm in favor the the `//` usage for formatting and making it look cleaner.. without them it seems
spotless will re-format these lines.
application/org.openjdk.jmc.feature.joverflow.ext.treemap/feature.xml line 56:
> 55:
> 56: <plugin
> 57: id="org.openjdk.jmc.joverflow.ext.treemap"
indented a tab here instead of spaces
application/org.openjdk.jmc.joverflow.ext.treemap/plugin.xml line 39:
> 38: <extension point="org.eclipse.ui.views">
> 39: <category name="JOverflow" id="JOverflow"/>
> 40: <view
tab indented on lines 39, 40, 48, 49
application/org.openjdk.jmc.joverflow.ext.treemap/src/main/java/org/openjdk/jmc/joverflow/ext/treemap/TreemapPage.java
line 70:
> 69: /* package-private */ class TreemapPage extends Page implements ModelListener {
> 70: private static final Color[] COLORS = {new Color(Display.getCurrent(), 250, 206, 210), // red
> 71: new Color(Display.getCurrent(), 185, 214, 255), // blue
Note: I thought this line looked a bit "off" with the color directly proceeding the `{`, but this seems to be the way
spotless wants it to be formatted. Just a heads up to anyone else checking this out...
application/org.openjdk.jmc.joverflow.ext.treemap/src/main/java/org/openjdk/jmc/joverflow/ext/treemap/TreemapAction.java
line 69:
> 68: ZOOM_OUT(Messages.TreemapAction_ZOOM_OUT_DESCRIPTION, IAction.AS_PUSH_BUTTON, CoreImages.ZOOM_OUT), //
> 69: ZOOM_OFF(Messages.TreemapAction_ZOOM_OFF_DESCRIPTION, IAction.AS_PUSH_BUTTON, CoreImages.ZOOM_OFF);
> 70:
For the zoom off action, I feel like the icon should closer represent something to a "reset" icon, maybe like the one
JOverflow uses? Otherwise it's not immediately obvious what the button does without reading it's tooltip.
application/org.openjdk.jmc.joverflow.ext.treemap/src/main/java/org/openjdk/jmc/joverflow/ext/treemap/TreemapPageBookView.java
line 114:
> 113: new TreemapAction(TreemapAction.TreemapActionType.ZOOM_OUT), //
> 114: new TreemapAction(TreemapAction.TreemapActionType.ZOOM_OFF), //
> 115: };
trailing comma
application/org.openjdk.jmc.joverflow.ext.treemap/src/main/java/org/openjdk/jmc/joverflow/ext/treemap/swt/TreemapToolTip.java
line 55:
> 54: protected Composite createToolTipContentArea(Event event, Composite parent) {
> 55: Composite ret = new Composite(parent, SWT.NONE);
> 56:
minor nit regarding variable name choice, is ret supposed to be rect?
application/org.openjdk.jmc.joverflow.ext.treemap/src/main/java/org/openjdk/jmc/joverflow/ext/treemap/swt/Breadcrumb.java
line 350:
> 349: */
> 350: public BreadcrumbItem getSelection() {
> 351: checkWidget();
does this get called anywhere?
-------------
PR: https://git.openjdk.java.net/jmc/pull/77
More information about the jmc-dev
mailing list