Review Request for JMC-6467

[Alex Macdonald]

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)


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