RFR(XL): 8199712: Flight Recorder

[Markus Gronlund]

Hi Coleen,

thanks for taking a look.

We have filed https://bugs.openjdk.java.net/browse/JDK-8202578  to follow up on the placement of where to post the class unload events.

About that, it might be possible to at least move the actual post_class_unload_event() function to InstanceKlass::notify_unload_class() as you suggest, with the proviso that we insert the "unloading hook", Jfr::on_unloading_classes(), just after the classes_do(InstanceKlass::notify_unload_class). Another problem, maybe minor, with moving the posting of class unload events to InstanceKlass::notify_unload_class() is how to maintain the same timestamp for each individual unload event.

The posting for unloads is "two-stage" in that the first pass fires an unload event (instant, same timestamp) for each klass; the second stage enters the JFR framework via on_unloading_classes() to save all constants for these classes now about to disappear. I think we could do that though, something like this:

…
  // Tell serviceability tools these classes are unloading
  classes_do(InstanceKlass::notify_unload_class);
  
  JFR_ONLY(Jfr::on_unloading_classes();)
...

About also getting post_class_load_event() and post_class_define_event() to InstanceKlass, this will be more problematic. The class load event is a duration event, in that it measures the actual time it took to load the class (it is stack allocated inside SystemDictionary::resolve_instance_class_or_null()). Therefore it might not be possible to move it. Maybe for define_event() as it is only an instant event.

We can sort out the intricacies involved in the context of https://bugs.openjdk.java.net/browse/JDK-8202578 .

About headers and include guards:

There is some inconsistency here, as it is not obvious what needs to be guarded at the include site.

We have reworked this a bit (will be included in updated webrev) according to the following:

Inclusion of headers for event programming can be unconditionally included, and the INCLUDE_JFR definition evaluation will happen in the header itself. The main reason is to support event programming without explicit conditionals (around the event code itself). The machine generated event code will have a dual, an empty stub, for every event that is outside INCLUDE_JFR.

This applies mainly to the following pattern:

// unconditionally included for access to event class
...
#include "jfr/jfrEvent(s).hpp
...

Other headers (support headers) will be changed where possible to not have the INCLUDE_JFR guard inside them unless absolutely needed (only for additional code that need empty stubs).
This will push the include condition to the include site, which at least is similar to what is done in other code, which is verbose albeit somewhat consistent.

That means we will need to add sections such as:
...
#if INCLUDE_JFR
#include "someheader.hpp"
#endif

An unfortunate side-effect of this is that some sites (esp. in .hpp's) will now need dual macros like:

...
#if INCLUDE_JFR
#include "someheader.hpp"
#endif

  // Since the above is now under an include guard, this will need to follow:
  JFR_ONLY(DEFINE_FIELD;)
  
This is now macro-on-macro...

Maybe the above case is a situation where nested INCLUDE_JFR with stubs is warranted.

Moving forward maybe we will be able to move away from this macro:fied code for improved readability (depending on what we need for minimalVM).

Thanks again
Markus

-----Original Message-----
From: Coleen Phillimore 
Sent: den 26 april 2018 19:59
To: hotspot-dev at openjdk.java.net
Subject: Re: RFR(XL): 8199712: Flight Recorder

http://cr.openjdk.java.net/~egahlin/8199712.0/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html

We can file another RFE for this but I think you could call
post_class_unload_event() from in InstanceKlass and call from inside of InstanceKlass::notify_unload_class.

void ClassLoaderData::unload() {
   _unloading = true;

   // Tell serviceability tools these classes are unloading
   classes_do(InstanceKlass::notify_unload_class);

Rather than walking through _klasses again during unloading.  I think we should see if this is possible to improve this after this checkin.

http://cr.openjdk.java.net/~egahlin/8199712.0/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html

Then move post_class_load and post_class_define events to instanceKlass.cpp too.

http://cr.openjdk.java.net/~egahlin/8199712.0/src/hotspot/share/utilities/vmError.cpp.udiff.html

Sometimes files are #included inside of #if INCLUDE_JFR and sometimes they aren't.   Should jfr/jfr.hpp have the #if INCLUDE_JFR inside of it?

I reviewed all the shared code in directories classfile, runtime, oops, utilities, except for utilities/ticks.hpp changes, which require an expert for that.

It looks like you have my last change that was in jfrArtifacts.cpp now in

new/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeManager.hpp

Great!

Thanks,
Coleen


On 4/25/18 7:06 AM, Erik Gahlin wrote:
> Greetings,
>
> Could I have a review of 8199712: Flight Recorder
>
> As mentioned in the preview [1] the tracing backend has been removed. 
> Event metadata has been consolidated into a single XML file and event 
> classes are now generated by GenerateJfrFiles.java.
>
> Tests have been run on Linux-x64, Windows-x64 and MaxOSX-x64.
>
> For details about the feature, see the JEP:
> https://bugs.openjdk.java.net/browse/JDK-8193393
>
> Webrev:
> http://cr.openjdk.java.net/~egahlin/8199712.0/
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8199712
>
> [1]
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-April/031359.h
> tml
>
> Thanks
> Erik and Markus