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.bate...@oracle.com>; hotspot-jfr-...@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
