[Preliminary Review]: Proposal for back-porting JFR to OpenJDK8u

[Andrey Petushkov]

Hi guys,

> On 1 Mar 2019, at 15:59, Mario Torre <neugens at redhat.com> wrote:
> 
> On Fri, Mar 1, 2019 at 1:38 PM guangyu.zhu <guangyu.zhu at aliyun.com> wrote:
>> 
>> Hi Mario, Andrey
>> 
>> Very happy to see things moving forward. Some of my updates:
> 
> Thanks for your work and help!
> 
>> As Andrey and I both agreed, in order not to waste resources, Alibaba will be responsible for merging the missing features. The work is in progress, my colleague Denghui and I are working on below tasks.
>> 
>> - thread sampling. [code completed, under testing]
>> - biased locking events [code completed, under testing]
That's great! Thank you!
>> - G1 heap summary and region types  [not started yet]
>> - -XX:+/-EnableJFR run time flag [not started yet]
>> 
>> For the first three missing features, once we complete the coding and testing, we will send out webrev. For the last one, we added an 'EnableJFR' flag to enable or disable JFR functionality at runtime. Jdk11 does not have such a runtime flag. So we didn't start working right away, we want to hear the opinions of the community first.
> 
> I don't think we should diverge in any way from the other versions
> unless necessary. For 8u-dev I would like to follow what Oracle did on
> their VM, with the usual pack of -XX:+UnlockCommercialFeatures
> -XX:+FlightRecorder -XX:StartFlightRecording=xxx options.
> 
> I know -XX:+UnlockCommercialFeatures isn't necessary in our case, but
> it should be there for compatibility. I would be ok if the option
> didn't do anything but was just there as to not crash the VM at
> startup when someone was migrating from one downstream version to the
> other. Of course, it can be said that -XX:+UnlockCommercialFeatures in
> OpenJDK currently doesn't exist and is deprecated in later versions
> anyway, so I defer to Andrew Haley regarding this specific point if he
> has a strong opinion of not introducing the option at all.
I fully agree that UnlockCommercialFeatures flag should be left for compatibility. That's exactly
what I have done, added it with a help text that this is the only purpose. In fact without it
one cannot use JMC with jdk8, JMC has built-in knowledge about necessity to specify
this flag if it deals with version 8
> 
> Regarding +EnableJFR as a runtime flag, my take is that
> -XX:+FlightRecorder -XX:StartFlightRecording are already doing the
> same.
Agree. Also, having another flag which is false by default will prevent from using typical procedure
to connect JMC to such JFR implementation. One would need to either EnableJFR with command line
or management interface beforehand
> 
>> There is one thing we think should be mentioned. Azul's patch removes the trace feature (JEP 167: Event-based JVM tracing, am I correct?) and replaces it with jfr, which is the same as jdk11. I am not sure if anyone is relying on the original trace feature. Maybe we should keep it. What do you think?
> 
> I'm going through the patch right now, but yes, from what I see the
> trace is removed. I had too a concern about this and was about to send
> a note. I'm not quite sure what to do, because Trace has been removed
> in 11 as far as I know, but removing mid stream in 8 may be a more
> interesting issue, however this isn't a user facing API, it was always
> meant to be internal to the JVM, so I don't quite know if there's
> really a reason we shouldn't change it. This is one question for the
> CSR group I think.
Since trace was removed by the same commit as JFR was added to jdk11 my guess is that trace 
was used internally at Oracle to integrate closed implementation of JFR. With this sense I see no point
to keep it. However if the guess is wrong and there some alternative implementation of trace event consumer
I will be happy to return it back

Thanks,
Andrey
> 
> Cheers,
> Mario
> -- 
> Mario Torre
> Associate Manager, Software Engineering
> Red Hat GmbH <https://www.redhat.com>
> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898