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

Hi Alan, Some context might be helpful here: The real JFR API is located in module jdk.jfr and the API handle there is this class jdk.jfr.Event [1] There are multiple reasons for choosing an class as the API instead of an interface - most of them are driven by implementation (in the VM) and performance related. As you can see in [1], the methods we are discussing here are there declared "final". This reflects the fact that we don't want user versions of these methods. We do want the signatures and the schema however so we can rework classes and methods in the VM. Using a class hierarchy allows us via induction quickly decide (bit check) if a loading class is related to the API (which cannot be done as performant should the solution be based on an interface) and we want to work with invokevirtual and invokespecial but not so much invokeinterface. So this solution we are discussing here is about how we can extend what we already have in place for JFR to also allow code in java.base to use it. The main challenge here is with solving the module system inversion in that java.base can't have a compile dependency on the real JFR API located in jdk.jfr: With the solution proposed by Erik, we extend our JFR VM machinery to also treat jdk.internal.events.Event in the same way as jdk.jfr.Event. Unfortunately, in order to do this, we now can't make the methods in jdk.internal.events.Event "final" since we will need inherit it from jdk.jfr.Event (which implement these methods already in the API proper). Also, there now needs to be careful means in the VM of avoiding jdk.jfr module related symbols when processing java.base events in certain states (for example, a module read link not having been established (yet or at all) etc). I don’t think we should view jdk.internal.events.Event as an general API but instead it is a means of hooking into the JFR system at runtime, via the VM, avoiding the compile time dependency. "Regular code" should work with the jdk.jfr module proper if possible, but for situations that cannot handle that / don't want to add the module import for some reason, this is a JDK internal hook point, that can be used if needed. This is also the reason why it is not exported to other modules in general, it is mainly intended for java.base, since the inversion leave no other alternatives (except moving all jfr code into java.base which I think is probably not an option). In addition, nothing in jdk.internal.events.Event strongly couple with jdk.jfr which means it may be put to some other use for alternative implementations that do not have a JFR implementation say. Hope this clarifies a bit Thanks Markus [1] https://docs.oracle.com/javase/9/docs/api/jdk/jfr/Event.html -----Original Message----- From: Alan Bateman Sent: den 21 juni 2018 09:52 To: Erik Gahlin <erik.gahlin@oracle.com>; hotspot-jfr-dev@openjdk.java.net; core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr On 20/06/2018 18:59, Erik Gahlin wrote:

:

...

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 I'm not sure that I understand the issues here but just to say that jdk.internal.event is not exported so code outside of the JDK cannot directly instantiate instances of classes in that package. Also interfaces can have default methods which may go to your concern about needing to implement each of the 6 methods. Once Event is cleaned up with some javadoc then it will be easier to argue this one way or the other.

-Alan

On 24/06/2018 12:42, Erik Gahlin wrote:

I have updated jdk.internal.event.Event class with comments.

http://cr.openjdk.java.net/~egahlin/8203629.2/

It is basically a copy the comments in jdk.jfr.Event, but without reference to classes in the JFR module.

The rationale for using a class instead of an interface has to do with the implementation of JFR. Markus brought up some reasons in his e-mail.

I thought about mentioning JFR in the javadoc, and that the purpose of the class is to allow use of JFR without a compile time dependency on the JFR module, but decided not to, since I think the class could have a value on its own for JVMs that do not support JFR, but that want to do to something similar for the JDK. They could, for instance, add an implementation in the empty method bodies. I read the mail from Markus and looked at the updated webrev. It's good to have some javadoc, just a bit strange to have an abstract class without any abstract methods. There isn't enough in the discussion here to understand the original rational for why jdk.jfr.Event is abstract, esp. as the constructor is protected so it can't be directed instantiated outside of the jdk.jfr package. Also an abstract class can extend a non-abstract class so abstract jdk.jfr.Event could extend a non-abstract jdk.internal.event.Event if you want to clean this up a bit. I don't want to get in the way of the current effort but at some point I think another set of eyes on the APIs here might help.

BTW: My previous comment about the modifiers was mostly about jdk.jfr.Event. -Alan

Hi Erik, I think this looks good. Thanks Markus -----Original Message----- From: Erik Gahlin Sent: den 24 juni 2018 21:16 To: Alan Bateman <alan.bateman@oracle.com>; hotspot-jfr-dev@openjdk.java.net; core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr On 2018-06-24 19:00, Alan Bateman wrote:

On 24/06/2018 12:42, Erik Gahlin wrote:

...

I have updated jdk.internal.event.Event class with comments.

http://cr.openjdk.java.net/~egahlin/8203629.2/

It is basically a copy the comments in jdk.jfr.Event, but without reference to classes in the JFR module.

The rationale for using a class instead of an interface has to do with the implementation of JFR. Markus brought up some reasons in his e-mail.

I thought about mentioning JFR in the javadoc, and that the purpose of the class is to allow use of JFR without a compile time dependency on the JFR module, but decided not to, since I think the class could have a value on its own for JVMs that do not support JFR, but that want to do to something similar for the JDK. They could, for instance, add an implementation in the empty method bodies. I read the mail from Markus and looked at the updated webrev. It's good to have some javadoc, just a bit strange to have an abstract class without any abstract methods.

The design stems from jdk.jfr.Event. If methods in jdk.jfr.Event would be abstract, it would not be possible to declare them final and we want them to be final, as users are not supposed to override them. When the jdk.jfr.Event class is loaded, the final modifiers are removed, so we can generate bytecode for subclasses. This is a trick to prevent incorrect use of the API. Why not make the class non-abstract and rely on the protected constructor? We could do that, it would prevent direct instantiation, but the use of the abstract modifier also fits nicely with how the modifier is used for event hierarchies. Imagine the following hierarchy and reuse of the sql field: class SQLEvent extends jdk.jfr.Event { @Label("SQL"); String sql; } class SQLUpdateEvent extends SQLEvent { } class SQLInsertEvent extends SQLEvent { } class SelectEvent extends SQLEvent { } When users connect with JMC, or call FlightRecorder#getEventTypes(), they will get a list of all event types that have been registered in the JVM. In the above example, they would get the SQLEvent, which is not really meant to be an event that users can configure. If they declare SQLEvent abstract, it will be ignored by the JFR system. This reuse mechanism can be seen in jdk.jfr.events.AbstractJDKEvent Why not use @Registered(false) instead of the abstract modifier? We want that annotation to be inherited, so it is possible to use it for large set events in a hierarchy. The abstract modifier only operates on a particular class. If the jdk.jfr.Event class is also declared abstract, it can reuse the same mechanism. It is convenient from an implementation point of view, no special case for jdk.jfr.Event, and it may also hint to a user that this the class is not something they should instantiate. It also prevents instantiation using reflection and setAccessible(true). An interface could also prevent instantiation, but it may open for malicious implementations and it seems wrong to make into an interface when users are not really meant to implement it. It also doesn't work well with the JVM implementation.

There isn't enough in the discussion here to understand the original rational for why jdk.jfr.Event is abstract, esp. as the constructor is protected so it can't be directed instantiated outside of the jdk.jfr package. Also an abstract class can extend a non-abstract class so abstract jdk.jfr.Event could extend a non-abstract jdk.internal.event.Event if you want to clean this up a bit. I don't want to get in the way of the current effort but at some point I think another set of eyes on the APIs here might help.

Thanks Alan. We can revisit the design later. Erik

BTW: My previous comment about the modifiers was mostly about jdk.jfr.Event.

-Alan

Sounds good Sean. We will push this RFE when your enhancement is ready and reviewed. We like to avoid pushing this to JDK 11 unless it is being used in "the current train". Erik

Thanks for implementing this enhancement Erik. I think it'll prove to be popular given the functionality available in JFR recordings. I'd agree with Alan's comment that the Event class could be documented better to indicate its purpose.

I'm already using this enhancement to implement some new events in the JDK security libraries. I hope to get a review started real soon on that front.

Regards, Sean.

On 19/06/18 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.

Thanks Erik