Review Request for JMC-6467

Tue Aug 6 15:57:10 UTC 2019

Hi Ken,

Great feedback! I actually hadn't noticed the text boxes being editable! So
you're completely right in that that should be fixed.

Regarding the region visualizer I get what you mean, even most production
G1 heaps aren't easily visualized there. It's definitely a possibility to
create a new component that could switch between visualizing the entire
heap over the entire recording (maybe with distribution or some
downsampling) and one that shows individual regions as now. The current
functionality is mostly due to that component already existing and that it
was more performant than my own, earlier, attempt (that also showed region
utilization).

Cheers,
Henrik Dafgård

On Tue, 6 Aug 2019 at 17:26, Ken Dobson <kdobson at redhat.com> wrote:

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