Shenandoah Events

[Aleksey Shipilev]

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.

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

*) 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"

*) Keep the new line after #includes:

 #include "utilities/globalDefinitions.hpp"
-
+#if INCLUDE_SHENANDOAHGC
+#include "gc/shenandoah/shenandoahJfrSupport.hpp"
+#endif // INCLUDE_SHENANDOAHGC
 /**

*) Indenting, arguments should be at the same column:

+  JfrSerializer::register_serializer(TYPE_SHENANDOAHHEAPREGIONSTATE,
+                                      false,
+                                      true,
+                                      new ShenandoahHeapRegionStateConstant());

*) 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;
+}

Cheers,
-Aleksey