RFR 8221507: Implement JFR Events for Shenandoah

[Erik Gahlin]

Looks good.

In the tests, you could avoid the null check on the recording and just 
use try-with-resources.  It will close the recording automatically when 
it runs out of scope. No need to post updated webrev.

Thanks
Erik

> Thank you for the review again.
>
> Yes, not sure regarding backporting plans but I imagine it will be in 
> the future so seemed best to avoid anything that cause issues such as 
> Set.of().
>
> I also think the naming of the jfr's is fine as it's stored in a 
> folder named after the test which was easily found now that I'm aware 
> that the jfrs are already dumped.
>
> I believe I've addressed the rest of your recommended changes to the 
> tests, let me know if I've covered everything.
>
> http://cr.openjdk.java.net/~kdobson/eventswithtests/webrev/ 
> <http://cr.openjdk.java.net/%7Ekdobson/eventswithtests/webrev/>
>
> Thanks,
>
> Ken Dobson
>
> On Sun, May 12, 2019 at 8:28 PM Erik Gahlin <erik.gahlin at oracle.com 
> <mailto:erik.gahlin at oracle.com>> wrote:
>
>     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
>>         >>
>>         >
>>
>