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

[Andrey Petushkov]

Hi Guangyu,

By the moment I've read first two patches (thread sampling and biased locks). These look good to me
One very small note, I'd rather be verbose with the value of _trace_flag in thread.hpp. Just to prevent occasional use of the same value for another item of this enum in the (almost impossible) case that someone adds it

I need a bit more time to read through G1 heap region types one.
BTW, it looks I've forgotten to mention that Azul is also missing G1 heap occupancy percent data which is indeed supported by Alibaba. Would you be able to integrate it also, please

Thanks a lot,
Andrey

> On 14 Mar 2019, at 05:39, guangyu.zhu <guangyu.zhu at aliyun.com> wrote:
> 
> Ok. I look forward to the feedbacks from both of you.
> 
> Thanks,
> Guangyu
> ------------------------------------------------------------------
> Sender:Mario Torre <neugens.limasoftware at gmail.com>
> Sent At:2019 Mar. 13 (Wed.) 19:26
> Recipient:Andrey Petushkov <andrey at azul.com>
> Cc:guangyu.zhu <guangyu.zhu at aliyun.com>; jdk8u-dev <jdk8u-dev at openjdk.java.net>; denghui.ddh <denghui.ddh at antfin.com>
> Subject:Re: [Preliminary Review]: Proposal for back-porting JFR to OpenJDK8u
> 
> Thanks!
> 
> I’m currently traveling but I’ll offer my review as soon as I get back next week.
> 
> Cheers,
> Mario
> On Tue 12. Mar 2019 at 09:35, Andrey Petushkov <andrey at azul.com> wrote:
> Hi Guangyu,
> 
> Cool! Thank you so much! Will review changes ASAP
> The backporting is still in progress. There are quite a lot of changes to consider so we'll likely finish some time after
> April update release (mostly limited by testing resources capacity)
> 
> Regards,
> Andrey
> 
> > On 12 Mar 2019, at 16:29, guangyu.zhu <guangyu.zhu at aliyun.com> wrote:
> >
> > Hi Andrey, Mario
> >
> > Is there any progress in backporting? We have completed the patch for the missing features. Please review.
> >
> > - thread sampling:
> > http://cr.openjdk.java.net/~luchsh/thread_sampling/
> >
> > - biased locking events:
> > http://cr.openjdk.java.net/~luchsh/hs_biasedlock/
> > http://cr.openjdk.java.net/~luchsh/jdk_biasedlock/
> >
> > - G1 heap region (heap summary is still missing, Alibaba's patch does not support heap summary either):
> > http://cr.openjdk.java.net/~luchsh/g1region_type_change_event
> >
> > Thanks,
> > Guangyu
> >
> >
> > ------------------------------------------------------------------
> > Sender:Andrey Petushkov <andrey at azul.com>
> > Sent At:2019 Mar. 6 (Wed.) 00:34
> > Recipient:Mario Torre <neugens at redhat.com>
> > Cc:guangyu.zhu <guangyu.zhu at aliyun.com>; jdk8u-dev <jdk8u-dev at openjdk.java.net>; denghui.ddh <denghui.ddh at antfin.com>
> > Subject:Re: [Preliminary Review]: Proposal for back-porting JFR to OpenJDK8u
> >
> > Hi Mario,
> >
> > > On 4 Mar 2019, at 14:19, Mario Torre <neugens at redhat.com> wrote:
> > >
> > > On Fri, Mar 1, 2019 at 8:09 PM Andrey Petushkov <andrey at azul.com> wrote:
> > >
> > >>> 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
> > >
> > > Yes, I tend to agree with you, I do believe this is mostly an internal
> > > API for easy of patching with the JFR code (which is almost
> > > identical). The only concern is in the way the logging would be
> > > triggered externally and the compile time options for it (I still see
> > > a couple of instance where INCLUDE_TRACE is being used). As for
> > > triggering the logs, I don't recall that 8 has any means of doing
> > > this, I think some infrastructure came with 9 with the -Xlog option (I
> > > didn't follow this however, I'm not sure the option ever landed in 9)?
> > > In that case I guess it's safe to go after all.
> > Right, the new logging infrastructure is badly missing here. Both Alibaba and Azul have added means of
> > some JFR logging but far from what jdk11 could do. Let me check the rest of INCLUDE_TRACE places,
> > IMHO we should get rid of all of them, but cannot tell for sure now
> >
> > 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
> >
> 
>