RFR 8221507: Implement JFR Events for Shenandoah

[Erik Gahlin]

Thanks for fixing!

Product code looks good, but I think tests could be improved.

There is no need to call dump in a final clause.

Events.fromRecording(...) will dump the file into the jtreg scratch 
directory ( "recording-1-pid-xxx.jfr")  by default. I think this is 
sufficient since there is only one recording per test.  If you think the 
name is cryptic, consider adding another method to Event.fromRecording 
that takes an additional parameter that becomes a suffix, i.e 
"recording-1-pid-xxx-test-heap-region-info.jfr"

try (Recording r = new recording()) {
   r.enable(EVENT_NAME);
   r.start();
   r.stop()
   List<RecordedEvent> events = Events.fromRecording(r);
   Events.hasEvents(events);
   for (RecordedEvent event : events) {
     Events.assertField(event, "index").notEqual(-1);
     Events.assertField(event, "used").atMost(1024*1024L);
     String state = Events.assertField(event, "state").getValue();
     GCHelper.assertShenandoahHeapRegionState(state));
}

The name of the field in metadata.xml is "state", but the tests looks 
for "type". That seems incorrect.

GCHelper:

public static assertShenandoahHeapRegionState(String state) {
  if (!shenandoahHeapRegionStates.contains(state)) {
     throw new AssertionError("Unknown state '"+ state +"', valid heap 
region states are " + shenandoahHeapRegionStates);
}

I assume you don't use Set.of("Empty Uncommitted" , ... , "Trash") 
because you want to backport to JDK 8.

Thanks
Erik

> Thanks everyone for their reviews. I've added tests similar to the 
> tests for the G1 events as well as addressed the changes that Erik 
> recommended.
>
> Here is the new webrev: 
> http://cr.openjdk.java.net/~kdobson/shenandoaheventswithtests/webrev/ 
> <http://cr.openjdk.java.net/%7Ekdobson/shenandoaheventswithtests/webrev/>
>
> Please let me know if there's anything else that should be addressed.
>
> Thanks,
>
> Ken Dobson
>
> On Mon, May 6, 2019 at 11:38 AM <mikhailo.seledtsov at oracle.com 
> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>
>
>
>     On 5/4/19 2:16 PM, Erik Gahlin wrote:
>     > On 2019-05-04 00:19, mikhailo.seledtsov at oracle.com
>     <mailto:mikhailo.seledtsov at oracle.com> wrote:
>     >> Hi,
>     >>
>     >>   If possible and feasible, could you please implement tests
>     for new
>     >> events?
>     >>
>     >> Please place the tests under test/jdk/jdk/jfr/event/gc/. Perhaps
>     >> create a "shenandoah" subfolder.
>     >>
>     >> If the events are too hard to test or not feasible, please add the
>     >> event names to
>     >> test/jdk/jdk/jfr/event/metadata/TestLookForUntestedEvents.java, to
>     >> the hardToTestEvents set. Consider commenting why it is too
>     hard to
>     >> test.
>     >
>     > If events are are marked experimental, tests are ignored by
>     > TestLookForUntestedEvents. java, so we should not  add them to the
>     > hardToTestEvents set. That may help them pass under the radar
>     once the
>     > experimental attribute is removed.
>     Thank you. If the events are "experimental" this approach makes
>     sense.
>     Once the experimental status is removed, the
>     TestLookForUntestedEvents.java should be able to notice if the
>     event is
>     not covered by tests.
>     > That said, tests never hurts, even if they are experimental. I
>     think
>     > test/jdk/jdk/jfr/event/gc/detailed would be a good place
>     +1
>
>     Thank you,
>     Misha
>     >
>     > Erik
>     >
>     >>
>     >> Also, please make sure to run all jfr tests under
>     test/jdk/jdk/jfr/
>     >> prior to integration.
>     >>
>     >>
>     >> Best regards,
>     >>
>     >> Misha
>     >>
>     >>
>     >>
>     >> On 5/3/19 7:44 AM, Ken Dobson wrote:
>     >>> Hi all,
>     >>>
>     >>> Please review this patch that adds support for two new JFR events
>     >>> ShenandoahHeapRegionStateChange and
>     ShenandoahHeapRegionInformation.
>     >>>
>     >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221507
>     >>> Webrev: http://cr.openjdk.java.net/~kdobson/53476/webrev/
>     <http://cr.openjdk.java.net/%7Ekdobson/53476/webrev/>
>     >>>
>     >>> The Shenandoah team has also reviewed this patch and approved it
>     >>> from their
>     >>> end.
>     >>>
>     >>> Thanks,
>     >>>
>     >>> Ken Dobson
>     >>
>     >
>