Review Request for JMC-6467

Tue Aug 6 15:26:36 UTC 2019

Hi Henrik,

I tested it out and the functionality all looks good the only issue I found
was it was missing some of the Shenandoah states which cause some things to
break, I've added them in the webrev below.

I do have a couple comments regarding the UI:

1. It doesn't seem necessary for the legend names to be an editable text
box, I can't foresee any use for this and it's more likely to cause
confusion.
2. The Region Visualizer is unlikely to ever be able to be rendered for
Shenandoah due to the number of regions, I wonder if it might make sense
for the default chart to be more like a stack chart showing the total
distribution (i.e. 20% Empty Uncommitted 40% Regular 40% Trash) or
something of that nature. Any changes to the region visualizer are best for
another patch but it would be worth considering.

Webrev: http://cr.openjdk.java.net/~kdobson/JMC-6467/webrev/

Cheers,

Ken Dobson

On Wed, Jul 31, 2019 at 3:44 PM Alex Macdonald <almacdon at redhat.com> wrote:

> Hi Henrik,
>
> On Tue, Jul 30, 2019 at 11:04 AM Alex Macdonald <almacdon at redhat.com>
> wrote:
>
> > Hi Henrik,
> >
> > On Tue, Jul 30, 2019 at 7:45 AM Henrik Dafgård <hdafgard at gmail.com>
> wrote:
> >
> >> Hi Alex,
> >>
> >> Thanks for finding this, the issue seems to be that I mis-typed the type
> >> string for Jdk.ShenandoahRegionStateChange as
> >> Jdk.ShenandoahRegionTypeChange. Fixing that solved the NPEs.
> >>
> >> Updated Webrev: http://cr.openjdk.java.net/~hdafgard/JMC-6467/webrev.1/
> >>
> >>
> > Hm, it looks like I'm still encountering the same problem as before. It
> > wasn't that branch of the if-statement in G1Constants which was running
> > into problems (but good a typo was caught), but rather the one above it:
> >
> > [..]
> > } else if
> >
> (type.getIdentifier().equals(JdkTypeIDs.GC_SHENANDOAH_HEAP_REGION_INFORMATION))
> > {
> >   return REGION_STATE.getAccessor(type);
> > }
> > [..]
> >
> > The returned value here is null for me. When I take a look at the passed
> > type object it is correctly a jdk.ShenandoahHeapRegionInformation,
> however
> > the state attribute looks like:
> >
> > type.m_accessors[2] = Attribute(state,
> >
> Linear(number))=org.openjdk.jmc.common.unit.StructContentType$AccessorEntry at 5b32e0b1
> >
> > but the REGION_STATE is looking for PLAIN_TEXT ("text"), which is a
> > mismatch.
> >
> > For reference here's the stack trace that gets displayed in the jfr
> editor
> > page:
> >
> > java.lang.NullPointerException
> >   at
> >
> org.openjdk.jmc.flightrecorder.ext.g1.G1Page$G1PageUI.createRegionList(G1Page.java:443)
> >   at
> >
> org.openjdk.jmc.flightrecorder.ext.g1.G1Page$G1PageUI.setUpHeapDumps(G1Page.java:454)
> >   at
> >
> org.openjdk.jmc.flightrecorder.ext.g1.G1Page$G1PageUI.<init>(G1Page.java:218)
> >   at
> org.openjdk.jmc.flightrecorder.ext.g1.G1Page.display(G1Page.java:586)
> >   at
> >
> org.openjdk.jmc.flightrecorder.ui.JfrEditor.displayPage(JfrEditor.java:232)
> >   at
> >
> org.openjdk.jmc.flightrecorder.ui.JfrEditor.navigateTo(JfrEditor.java:216)
> >   at
> >
> org.openjdk.jmc.flightrecorder.ui.JfrOutlinePage.selectionChanged(JfrOutlinePage.java:449)
> >
>
> My mistake, it looks like the recording I've been using may be irrelevant
> because it was using a work-in-progress implementation of the shenandoah
> events rather than the final ones. Ken has supplied me a newer shenandoah
> recording and the page is visible and working now.
>
> As for some minor nits:
> There are a handful of empty spaces/tabs that can be cleaned up:
> - ColorMap (9)
> - G1Page (4)
> - TypeIDs (2)
>
> Also the page name has been generalized from "G1 Heap Layout", would now
> also be a good time to update the other instances of g1* throughout the
> extension?
>
>
> > Cheers,
> >> Henrik Dafgård
> >>
> >>
> >> On Mon, 29 Jul 2019 at 18:03, Alex Macdonald <almacdon at redhat.com>
> wrote:
> >>
> >>> Hi Henrik,
> >>>
> >>> On Thu, Jul 25, 2019 at 1:37 PM Henrik Dafgård <hdafgard at gmail.com>
> >>> wrote:
> >>>
> >>>> Hi all,
> >>>>
> >>>> Please review this request for generalizing the G1 Heap Layout page.
> The
> >>>> pages.xml file that contains the new default color values could use
> one
> >>>> or
> >>>> two suggestions for Shenandoah events, if anyone has particular
> >>>> suggestions
> >>>> for default color values there.
> >>>>
> >>>
> >>> I've applied the patch and tried to open the Heap Layout page, however
> >>> I'm running into a NPE when G1Page.createRegionList() [0] is adding
> items
> >>> to "allRegions".
> >>>
> >>> For type jdk.ShenandoahHeapRegionInformation, the "typeAccessor"
> >>> variable [1] is coming back null, which ultimately causes an NPE when
> >>> trying to to retrieve a value from it [2]. Taking a bit of a deeper
> look
> >>> there seems to be a mismatch between the structure of REGION_STATE and
> what
> >>> I'm observing in the list of accessors. G1Constants is looking for
> (state,
> >>> Type(text)), however the actual value of the key involving state is
> (state,
> >>> Linear(number)). To quickly test I was able to retrieve a value using
> >>> attr("state", "State", NUMBER), however this doesn't match the return
> type
> >>> of customAccessor().
> >>>
> >>> I only have one shenandoah jfr file that I've been using, if you aren't
> >>> experiencing this exception would you be able to provide a basic jfr
> file
> >>> for verifying the behaviour?
> >>>
> >>> [0]
> >>>
> http://hg.openjdk.java.net/jmc/jmc/file/91a803e741ac/application/org.openjdk.jmc.flightrecorder.ext.g1/src/main/java/org/openjdk/jmc/flightrecorder/ext/g1/G1Page.java#l442
> >>> [1]
> >>>
> http://hg.openjdk.java.net/jmc/jmc/file/91a803e741ac/application/org.openjdk.jmc.flightrecorder.ext.g1/src/main/java/org/openjdk/jmc/flightrecorder/ext/g1/G1Page.java#l453
> >>> [2]
> >>>
> http://hg.openjdk.java.net/jmc/jmc/file/91a803e741ac/application/org.openjdk.jmc.flightrecorder.ext.g1/src/main/java/org/openjdk/jmc/flightrecorder/ext/g1/G1Page.java#l458
> >>>
> >>>
> >>>> JIRA: https://bugs.openjdk.java.net/browse/JMC-6467
> >>>> Webrev: http://cr.openjdk.java.net/~hdafgard/JMC-6467/webrev.0/
> >>>>
> >>>>
> >>>> Cheers,
> >>>> Henrik Dafgård
> >>>>
> >>>
> >>> Cheers,
> >>>
> >>> Alex
> >>>
> >>
>