RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Erik Gahlin]

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