On 2018-06-20 19:09, Alan Bateman wrote:
On 19/06/2018 03:06, Erik Gahlin wrote:
Hi,
Could I have a review of an enhancement that will make it possible to
add JFR events to java.base, and possibly other modules in the JDK,
without a compile time dependency on jdk.jfr.
Bug:
https://bugs.openjdk.java.net/browse/JDK-8203629
Webrev:
http://cr.openjdk.java.net/~egahlin/8203629.0
Testing:
Tests in test/jdk/jdk/jfr
The functionality is a prerequisite for an upcoming enhancement that
will add JFR events to the security libraries.
In short, jdk.jfr.Event (located in jdk.jfr) extends
jdk.internal.event.Event (located in java.base). Metadata for events,
such as labels and descriptions, are added using a mirror event class
located in jdk.jfr module. If the fields of the mirror class and the
event class don't match an InternalError is thrown.
To illustrate how the mechanism can be used, see the following
example (not meant to be checked in).
http://cr.openjdk.java.net/~egahlin/8203629.example
Since the implementation of jdk.internal.event.Event is empty, the
JIT will typically eliminate all traces of JFR functionality unless
Flight Recorder is enabled.
Adding a way to get events from java.base is good but I wonder if
jdk.internal.event.Event could be cleaned up before you push this. It
would be nice to have a class description and some minimal method
descriptions too.
I will add some comments.
Also all the methods are empty which makes me wonder if they should be
abstract (as the class is abstract) or whether it should be an interface.
Abstract is needed to prevent user from instantiating the class.
The methods need to have a body, otherwise event classes in java.base
would need to implement the methods, which would be cumbersome. We like
to use a class as it simplifies the implementation if we know all event
classes have a common ancestor with java.lang.Object as a parent.
so we can be guaranteed that the class
Some of the method modifiers are in unusual order and it would be good
to get those cleaned up too.
I will make the class "public abstract". The other modifiers looked OK
to me, but please let me know if there are other modifiers you want me
to change.
Erik
