Review Request for JMC-6467

[Alex Macdonald]

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