Shenandoah Events

[Ken Dobson]

Hi Aleksey,

Thank you for the review.

On Wed, Jan 16, 2019 at 3:40 PM Aleksey Shipilev <shade at redhat.com> wrote:

> Hi Ken,
>
> On 1/16/19 9:25 PM, Ken Dobson wrote:
> > Here's a new patch with all of the serialization moved to the
> > shenandoahJfrSupport files. Please give it a review when you get the
> chance.
>
> A brief look:
>
> *) Not sure why some paths have JFR_ONLY, and some do not. Try to
> configure build with
> --with-jvm-features=-jfr and build? Pretty sure you would need to put
> something like NO_JFR_RETURN
> to send_jfr_region_transition_event declaration.
>
> I think JFR_ONLY is for when you call just a single jfr related function
in a section of the code that will still be run with JFR removed. It
removes the code in that case. Is this what you mean by NO_JFR_RETURN?
I couldn't get it to build with jfr, however I just now saw that this was a
bug Zhengyu has created a patch for so I will try that now.

*) Include guards should not contain "_VM_" anymore, since the path does
> not include it:
>
> +#ifndef SHARE_VM_GC_SHENANDOAH_SHENANDOAHJFRSUPPORT_HPP
> +#define SHARE_VM_GC_SHENANDOAH_SHENANDOAHJFRSUPPORT_HPP
>
> Removed.

*) There are some whitespace changes in jfr/metadata/metadata.xml, are
> those intentional? Like these:
>
>    </Type>
> -
> +
>    <Event name="GCHeapSummary" category="Java Virtual Machine, GC, Heap"
> label="Heap Summary"
> startTime="false">
>
> ...or these:
>
> -  <Event name="DumpReason" category="Flight Recorder" label="Recording
> Reason"
> -         description="Who requested the recording and why"
> +  <Event name="DumpReason" category="Flight Recorder" label="Recording
> Reason"
> +         description="Who requested the recording and why"
>
> I believe this was an error in the original patch I've removed them.

> *) Keep the new line after #includes:
>
>  #include "utilities/globalDefinitions.hpp"
> -
> +#if INCLUDE_SHENANDOAHGC
> +#include "gc/shenandoah/shenandoahJfrSupport.hpp"
> +#endif // INCLUDE_SHENANDOAHGC
>  /**
>
>
Fixed.

*) Indenting, arguments should be at the same column:
>
> +  JfrSerializer::register_serializer(TYPE_SHENANDOAHHEAPREGIONSTATE,
> +                                      false,
> +                                      true,
> +                                      new
> ShenandoahHeapRegionStateConstant());
>
>
Fixed.

*) Pretty sure it would be cleaner if we introduced the set_state method
> instead, and do the event
> there. For example:
>
>     switch (_state) {
>      case _empty_uncommitted:
>        do_commit();
> +      set_state(_empty_committed);
>        return;
>      default:
>
> +void ShenandoahHeapRegion::set_state(RegionState to) {
> +#if INCLUDE_JFR // probably?
> +  EventShenandoahHeapRegionStateChange evt;
> +  evt.set_index(region_number());
> +  evt.set_start((uintptr_t)bottom());
> +  evt.set_used(used());
> +  evt.set_from(_state);
> +  evt.set_to(to);
> +  evt.commit();
> +#endif
> +  _state = to;
> +}
>
> I've added a set_state method now. The evt.should_commit() I've added
checks whether the event/jfr is enabled. This seems to be the standard
practice based on other events.

Please give this a review when you have the chance, I'll check to make sure
it causes no issues configuring without JFR.

Thanks,

Ken Dobson