RFR 8221507: Implement JFR Events for Shenandoah

[Ken Dobson]

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/

Thanks,

Ken Dobson

On Sun, May 12, 2019 at 8:28 PM Erik Gahlin <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/
>
> 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> wrote:
>
>>
>>
>> On 5/4/19 2:16 PM, Erik Gahlin wrote:
>> > On 2019-05-04 00:19, 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/
>> >>>
>> >>> The Shenandoah team has also reviewed this patch and approved it
>> >>> from their
>> >>> end.
>> >>>
>> >>> Thanks,
>> >>>
>> >>> Ken Dobson
>> >>
>> >
>>
>>
>