RFR: 8223147: JFR Backport (to jdk8u)

Wed May 8 12:24:05 UTC 2019

Hi Andrey,

There are a few things I wanted to point out about this patch.

Overall, it seems good to me, of course not being different than what
we already pre-reviewed. There are a couple of points however that we
need to address, I think they should be addressed before the patch is
in, but I'm open for discussions.

By default, when I compile without jfr (without --enable-jfr) there
should be no reference to JFR in the resulting image, but this is not
the case.

For example, in the resulting images/j2sdk-image/jre/lib library I can
still see jfr.jar and a jfr directory with the jfr templates next to
the rest of the libraries. I think this should be removed (a step at
the end of the compilation process will do I think, if it's too
cumbersome to not generate those in the first place).

What's more is that the presence of JFR sneaks into the JDK at
runtime. For example, the attached applications when run on a default
build of OpenJDK 8 produces the following exception:

Exception in thread "main" javax.management.RuntimeMBeanException:
java.lang.IllegalArgumentException: VM option "FlightRecorder" does
not exist
    at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.rethrow(DefaultMBeanServerInterceptor.java:839)
    at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.rethrowMaybeMBeanException(DefaultMBeanServerInterceptor.java:852)
    at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.invoke(DefaultMBeanServerInterceptor.java:821)
    at com.sun.jmx.mbeanserver.JmxMBeanServer.invoke(JmxMBeanServer.java:801)
    at CheckJFR.main(CheckJFR.java:15)
Caused by: java.lang.IllegalArgumentException: VM option
"FlightRecorder" does not exist
    at sun.management.HotSpotDiagnostic.getVMOption(HotSpotDiagnostic.java:83)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at sun.reflect.misc.Trampoline.invoke(MethodUtil.java:71)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at sun.reflect.misc.MethodUtil.invoke(MethodUtil.java:275)
    at com.sun.jmx.mbeanserver.ConvertingMethod.invokeWithOpenReturn(ConvertingMethod.java:193)
    at com.sun.jmx.mbeanserver.ConvertingMethod.invokeWithOpenReturn(ConvertingMethod.java:175)
    at com.sun.jmx.mbeanserver.MXBeanIntrospector.invokeM2(MXBeanIntrospector.java:117)
    at com.sun.jmx.mbeanserver.MXBeanIntrospector.invokeM2(MXBeanIntrospector.java:54)
    at com.sun.jmx.mbeanserver.MBeanIntrospector.invokeM(MBeanIntrospector.java:237)
    at com.sun.jmx.mbeanserver.PerInterface.invoke(PerInterface.java:138)
    at com.sun.jmx.mbeanserver.MBeanSupport.invoke(MBeanSupport.java:252)
    at javax.management.StandardMBean.invoke(StandardMBean.java:405)
    at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.invoke(DefaultMBeanServerInterceptor.java:819)
    ... 2 more

When running against the jdk8u-jfr-incubator build instead produces
"false". If I run with the -XX:+FlightRecorder I even get a "true"
although I then can't request a flight recorder via JMC or the mbean
interface, while a standard build would give me "Unrecognized VM
option 'FlightRecorder'" in this case.

I think we should fix this before merging (and please, do feel free to
add this small snippet of code as a test case in the patch).

I'll try to give more useful feedback as I keep on testing.

Cheers,
Mario

On Tue, May 7, 2019 at 3:52 PM Mario Torre <neugens at redhat.com> wrote:
>
> On Tue, May 7, 2019 at 1:20 PM Andrey Petushkov <andrey at azul.com> wrote:
> >
> > Hi All,
>
> Hi Andrey, thanks for the patch!
>
> > RFR for initial JFR backport to current state of jdk8u-jfr-incubator is here http://cr.openjdk.java.net/~apetushkov/8223147/
> > Please could you review and hopefully approve
>
> I'm going through it now, I'll get back to you as soon as possible!
>
> > The changes are the same as in initial review request [1] except two fixes were added for the bugs found after integration
> > of Alibaba's implementation of thread sampling (note that according to the plan Alibaba's patches are not included into this RFR)
> > The webrev includes bug ids for all changes backported under it, however these are not separated on per-file basis
>
> This is perfect, so we have proper tracking of the patches.
>
> > One process question (thus Andrew in To), the new webrev currently only includes the code which applies to jdk8u-jfr-incubator
> > repo, but what about aarch64 and aarch32 ports which are not available there? I don't see a reason to prevent them from
> > having JFR and there is code ready for that ([1]). How could we proceed with integration of it?
>
> You are right, it didn't occur to me that this code was not part of
> the repository, but this is because the staging repo is forked from
> jdk8u-dev which doesn't contain those ports.
>
> I think we should indeed backport the other changes, perhaps once
> everything is in mainline we can push into the shenandoah/arm/etc...
> repositories individual patches?
>
> 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

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