Review Request for JMC-5923: Add allocation pressure column to stacktrace view
Jie Kang
jkang at redhat.com
Thu Aug 8 15:07:56 UTC 2019
On Fri, Jul 26, 2019 at 10:15 AM Salman Siddiqui <sasiddiq at redhat.com> wrote:
>
> Hi all,
>
> Here is a webrev for the patch:
> https://cr.openjdk.java.net/~aptmac/JMC-5923/webrev.00/
Hi Salman,
Some comments below:
+ IItemCollection itemsCollection =
ItemCollectionToolkit.build(IteratorToolkit.toList(items.iterator(),
items.size()).stream());
+ IItemCollection allItemsCollection =
ItemCollectionToolkit.build(IteratorToolkit.toList(allItems.iterator(),
items.size()).stream());
Should it be allItems.size() in the second line?
+ SimpleArray<IItem> items = frame.getItems();
+ Branch branch = frame.getBranch();
+ SimpleArray<IItem> allItems = new
SimpleArray<IItem>(branch.getFirstFrame().getItems().elements());
+ for (StacktraceFrame f: branch.getTailFrames()) {
+ allItems.addAll(f.getItems().elements());
+ }
As with Joshua's comments, tests would be neat. Does the ratio of the
frame's items against the frame's, branch's first frame's items plus
the items of all the branch's tail frames match the "ratio of the size
of the allocations of a particular class to the size of total
allocations from that stacktrace frame." ? I'll try to read more into
the underlying classes it but if someone with more experience with the
Stacktrace classes could take a look at this I'd appreciate it as
well.
+ Double itemsAlloc =
itemsCollection.getAggregate(JdkAggregators.ALLOCATION_TOTAL).numberValue().doubleValue();
+ Double allItemsAlloc =
allItemsCollection.getAggregate(JdkAggregators.ALLOCATION_TOTAL).numberValue().doubleValue();
+ return String.format("%.1f %%",
(itemsAlloc / allItemsAlloc ) * 100);
+ } catch (NullPointerException e) { //
ALLOCATION_TOTAL is only available in Memory page
+ return "";
This comment seems strange to me. When I have a selection through the
Memory page, there are still elements in the stack trace view that
have "" for their allocation pressure column. That makes sense but
doesn't really match the comment here. Maybe the comment can be
rephrased?
Regards,
>
> Thanks,
> Salman
>
> On Thu, Jul 25, 2019 at 9:42 PM Salman Siddiqui <sasiddiq at redhat.com> wrote:
>
> > Hi all,
> >
> > Here is patch that adds a column to the stacktrace view to display
> > normalized allocation pressure. The allocation pressure is a ratio of the
> > size of the allocations of a particular class to the size of total
> > allocations from that stacktrace frame.
> >
> > Also, as per suggestion from Marcus, I have normalized the count that is
> > displayed in the stacktrace view (this is similar to what is already being
> > displayed in the tool tip for the count).
> >
> > JIRA issue: https://bugs.openjdk.java.net/browse/JMC-5923
> > Related GitHub issue: https://github.com/JDKMissionControl/jmc/issues/6
> >
> > Patch is below.
> >
> > Thanks,
> > Salman
> >
> >
> > diff --git
> > a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/messages/internal/Messages.java
> > b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/messages/internal/Messages.java
> > index 7b576f3..cd8234a 100644
> > ---
> > a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/messages/internal/Messages.java
> > +++
> > b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/messages/internal/Messages.java
> > @@ -435,6 +435,7 @@ public class Messages extends NLS {
> > public static String SELECT_RANGE_WIZARD_TEXT;
> > public static String SELECT_RANGE_WIZARD_TITLE;
> > public static String SELECT_RANGE_WIZARD_TO_MUCH_SELECTED_WARNING;
> > + public static String STACKTRACE_VIEW_ALLOC_PRESSURE_COLUMN_NAME;
> > public static String STACKTRACE_VIEW_COUNT_COLUMN_NAME;
> > public static String STACKTRACE_VIEW_DISTINGUISH_FRAMES_BY;
> > public static String STACKTRACE_VIEW_FRAME_GROUP_CHOOSE;
> > diff --git
> > a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/views/stacktrace/StacktraceView.java
> > b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/views/stacktrace/StacktraceView.java
> > index 2b51e80..9d96016 100644
> > ---
> > a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/views/stacktrace/StacktraceView.java
> > +++
> > b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/views/stacktrace/StacktraceView.java
> > @@ -92,10 +92,16 @@ import org.eclipse.ui.part.ViewPart;
> > import org.openjdk.jmc.common.IDisplayable;
> > import org.openjdk.jmc.common.IMCFrame;
> > import org.openjdk.jmc.common.IState;
> > +import org.openjdk.jmc.common.collection.IteratorToolkit;
> > import org.openjdk.jmc.common.collection.SimpleArray;
> > +import org.openjdk.jmc.common.item.IItem;
> > import org.openjdk.jmc.common.item.IItemCollection;
> > +import org.openjdk.jmc.common.item.IItemFilter;
> > +import org.openjdk.jmc.common.item.ItemFilters;
> > import org.openjdk.jmc.common.unit.UnitLookup;
> > import org.openjdk.jmc.common.util.StateToolkit;
> > +import org.openjdk.jmc.flightrecorder.jdk.JdkAggregators;
> > +import org.openjdk.jmc.flightrecorder.jdk.JdkFilters;
> > import org.openjdk.jmc.flightrecorder.stacktrace.FrameSeparator;
> > import
> > org.openjdk.jmc.flightrecorder.stacktrace.FrameSeparator.FrameCategorization;
> > import org.openjdk.jmc.flightrecorder.stacktrace.StacktraceFormatToolkit;
> > @@ -181,7 +187,7 @@ public class StacktraceView extends ViewPart
> > implements ISelectionListener {
> > private static final Color COUNT_COLOR = SWTColorToolkit.getColor(new
> > RGB(100, 200, 100));
> > private static final String SIBLINGS_IMG_KEY = "siblingsColor";
> > //$NON-NLS-1$
> > private static final Color SIBLINGS_COUNT_COLOR =
> > SWTColorToolkit.getColor(new RGB(170, 250, 170));
> > - private static final int[] DEFAULT_COLUMN_WIDTHS = {700, 150};
> > + private static final int[] DEFAULT_COLUMN_WIDTHS = {700, 150, 150};
> > private static final String THREAD_ROOT_KEY = "threadRootAtTop";
> > //$NON-NLS-1$
> > private static final String FRAME_OPTIMIZATION_KEY =
> > "distinguishFramesByOptimization"; //$NON-NLS-1$
> > private static final String FRAME_CATEGORIZATION_KEY =
> > "distinguishFramesCategorization"; //$NON-NLS-1$
> > @@ -491,7 +497,7 @@ public class StacktraceView extends ViewPart
> > implements ISelectionListener {
> > new StacktraceViewToolTipSupport(viewer);
> > MCContextMenuManager mm =
> > MCContextMenuManager.create(viewer.getControl());
> > CopySelectionAction copyAction = new CopySelectionAction(viewer,
> > - FormatToolkit.selectionFormatter(stackTraceLabelProvider,
> > countLabelProvider));
> > + FormatToolkit.selectionFormatter(stackTraceLabelProvider,
> > countLabelProvider, allocPressureLabelProvider));
> > InFocusHandlerActivator.install(viewer.getControl(), copyAction);
> > mm.appendToGroup(MCContextMenuManager.GROUP_EDIT, copyAction);
> > mm.appendToGroup(MCContextMenuManager.GROUP_EDIT,
> > CopySettings.getInstance().createContributionItem());
> > @@ -515,6 +521,8 @@ public class StacktraceView extends ViewPart
> > implements ISelectionListener {
> > .setLabelProvider(stackTraceLabelProvider);
> > buildColumn(viewer, Messages.STACKTRACE_VIEW_COUNT_COLUMN_NAME,
> > SWT.RIGHT, columnWidths[1])
> > .setLabelProvider(countLabelProvider);
> > + buildColumn(viewer, Messages.STACKTRACE_VIEW_ALLOC_PRESSURE_COLUMN_NAME,
> > SWT.RIGHT, columnWidths[2])
> > + .setLabelProvider(allocPressureLabelProvider);
> >
> > PlatformUI.getWorkbench().getHelpSystem().setHelp(viewer.getControl(),
> > HELP_CONTEXT_ID);
> >
> > @@ -731,7 +739,13 @@ public class StacktraceView extends ViewPart
> > implements ISelectionListener {
> > private final ColumnLabelProvider countLabelProvider = new
> > ColumnLabelProvider() {
> > @Override
> > public String getText(Object element) {
> > - return Integer.toString(((StacktraceFrame) element).getItemCount());
> > + // display normalized count (percentage of total count)
> > + StacktraceFrame frame = (StacktraceFrame) element;
> > + Fork rootFork = getRootFork(frame.getBranch().getParentFork());
> > + int itemCount = frame.getItemCount();
> > + int totalCount = rootFork.getItemsInFork();
> > + return UnitLookup.PERCENT_UNITY.quantity(itemCount / (double) totalCount)
> > + .displayUsing(IDisplayable.AUTO);
> > }
> >
> > @Override
> > @@ -756,6 +770,31 @@ public class StacktraceView extends ViewPart
> > implements ISelectionListener {
> > }
> > };
> >
> > + private final ColumnLabelProvider allocPressureLabelProvider = new
> > ColumnLabelProvider() {
> > + @Override
> > + public String getText(Object element) {
> > + StacktraceFrame frame = (StacktraceFrame) element;
> > +
> > + SimpleArray<IItem> items = frame.getItems();
> > + Branch branch = frame.getBranch();
> > + SimpleArray<IItem> allItems = new
> > SimpleArray<IItem>(branch.getFirstFrame().getItems().elements());
> > + for (StacktraceFrame f: branch.getTailFrames()) {
> > + allItems.addAll(f.getItems().elements());
> > + }
> > +
> > + IItemCollection itemsCollection =
> > ItemCollectionToolkit.build(IteratorToolkit.toList(items.iterator(),
> > items.size()).stream());
> > + IItemCollection allItemsCollection =
> > ItemCollectionToolkit.build(IteratorToolkit.toList(allItems.iterator(),
> > items.size()).stream());
> > +
> > + try {
> > + Double itemsAlloc =
> > itemsCollection.getAggregate(JdkAggregators.ALLOCATION_TOTAL).numberValue().doubleValue();
> > + Double allItemsAlloc =
> > allItemsCollection.getAggregate(JdkAggregators.ALLOCATION_TOTAL).numberValue().doubleValue();
> > + return String.format("%.1f %%", (itemsAlloc / allItemsAlloc ) * 100);
> > + } catch (NullPointerException e) { // ALLOCATION_TOTAL is only available
> > in Memory page
> > + return "";
> > + }
> > + }
> > + };
> > +
> > private final ColumnLabelProvider stackTraceLabelProvider = new
> > ColumnLabelProvider() {
> >
> > @Override
> > diff --git
> > a/application/org.openjdk.jmc.flightrecorder.ui/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties
> > b/application/org.openjdk.jmc.flightrecorder.ui/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties
> > index cd954bd..96ce600 100644
> > ---
> > a/application/org.openjdk.jmc.flightrecorder.ui/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties
> > +++
> > b/application/org.openjdk.jmc.flightrecorder.ui/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties
> > @@ -418,6 +418,7 @@ STORED_SELECTIONS_SIZE_UNPARSABLE=Number of stored
> > selections to keep: {0}
> > STORE_SELECTION_ACTION=Store Selection
> > STORE_AND_ACTIVATE_SELECTION_ACTION=Store and Set As Focused Selection
> >
> > +STACKTRACE_VIEW_ALLOC_PRESSURE_COLUMN_NAME=Allocation Pressure
> > STACKTRACE_VIEW_COUNT_COLUMN_NAME=Count
> > STACKTRACE_VIEW_DISTINGUISH_FRAMES_BY=Distinguish Frames By
> > STACKTRACE_VIEW_FRAME_GROUP_CHOOSE=Choose Frame Group
> >
> >
> >
More information about the jmc-dev
mailing list