[RFC]: Integrate jdk8u-jfr-incubator with jdk8u-dev

Mon Feb 24 12:56:12 UTC 2020

Hi Andrew,

Thank you very, very much for this review, I appreciate you taking the
time to look at the proposed changes.

Regarding the possible compatibility you mentioned due to the mbean
interface addition, we did file a CSR for this backport, I think I
didn't mention this in my original email so here is the link to it:

https://bugs.openjdk.java.net/browse/JDK-8230764

I agree there may be a very low risk of breaking some applications,
one of our reasoning during the review process the jdk.management.jfr
is not a namespace in JDK8u, so effectively this will appear to
applications as any other custom mbean name that may eventually be
provided by the client code, I assume that only applications that may
scan through the list of available mbeans will notice, but given the
namespace is not protected in 8u it won't matter: the bean name won't
be available when JFR is not compiled in, and in this case an
exception will be thrown, but this is inline with the current
specification for all mbeans names when not available.

I don't know what the current closed JDK does regarding the mbeans
interfaces, I guess in 8 they have a different name, so this maybe an
area where application may need to look for two names when trying to
enable JFR via JMX, on the other hand, nothing changes for
applications that already run on 8 now but don't use JFR on OpenJDK.
JDK Mission Control is aware of this however, and is able to start
flight recorder without problems, so any application that uses this
tool, or uses jcmd, will be automatically fine.

Given that jdk.management.jfr is also the standard namespace in 11+ we
have the benefit that applications will be compatible across all
versions now. We also retained the UnlockCommercialFeatures flag for
compatibility (however it does nothing of course).

Thanks again for your time and review!

Cheers,
Mario

On Mon, Feb 24, 2020 at 12:58 PM Andrew Dinn <adinn at redhat.com> wrote:
>
> Hi Mario,
>
> n.b. I'm replying to this RFC thread on the grounds that it appears to
> be the de facto RFR thread for the patch. So,m take this as a final
> review for whether or not to include the patch into jdk8u-dev. In sum,
> it's a yes.
>
>
> I have reviewed (by eyeball) the latest set of patches for inclusion of
> JFR into jdk8u-dev, concentrating on the JVM code in particular. The
> patch version I used is:
>
>   http://cr.openjdk.java.net/~neugens/JDK8u-JFR.04/
>
> The effect on jdk8u is considered in 3 different aspects below. I don't
> think any aspect leads to cause for concern over committing the backport.
>
> 1) Isolation of changes when JFR is excluded at build time
>
> Most of the changes to JVM code are only conditionally compiled into the
> build if enabled at build i.e. with --enable-jfr. However, the patch
> does still involve changes to the JVM code base that will be compiled
> into the product even without that option. I am confident that all such
> changes are trivial and will not affect the behaviour of the JVM.
>
> The same conclusion holds for the JDK code. The few changes to JDK
> runtime code are small additions to the API surface and underlying
> behaviour. I do not believe they will affect code that is not using the
> new APIs.
>
> 2) Isolation of changes when JFR is included at build time.
>
> 2a) Effect on Runtime and App Behaviour
>
> As far as functionality is concerned I could only find one circumstance
> in the JDK code where the presence of JFR might result in noticeably
> different operation of the application. Code which processes MXBeans may
> observe the new JFR PlatformMBean and not recognise it. I think that is
> a low risk issue and delaying the backport is not going to mitigate it
> significantly (we ca't avoid this possibility and won't find the problem
> cases until we release a backported version).
>
> Other JDK and JVM changes may result in different operation in the
> underlying called code (e.g. existing trace code has been replaced by
> JFR code) but I don't believe these differences will change the way the
> caller operates i.e. I believe they will be semantically neutral to clients.
>
> 2b) Effect on Runtime and App performance
>
> I could not find any reason to expect any significant performance
> differences when JFR is included but not started, whether by omitting to
> start on the command line or via jcmd.
>
> The only two points where extra work might be done on a regular basis --
> and therefore where a significant impact might be seen on performance --
> are the leak profiler (called regularly by the GC) and the various event
> dispatch calls scattered throughout the JVM code. Both of these short
> circuit without doing any work unless flight recorder is started either
> on the command line or via jcmd so the cost is unlikely to be noticed.
> The event dispatch does involve a small amount of preparatory work
> (allocate and initialize an event on the stack, test whether it is
> enabled) but the C compiler ought to optimize most of this cost away and
> even if it doe snot it should not add much overhead.
>
> Of course, there will be an impact on performance when JFR is in use and
> this may be noticeable. I could not identify any reason to expect the
> cost of running with JFR on to be significantly higher than when it is
> used on jdk11.
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill
>
> On 30/01/2020 11:37, Mario Torre wrote:
> > Hi all,
> >
> > The moment has finally arrived where we are at the point to request
> > the big merge.
> >
> > Andrew Hughes was kind enough to provide the webrevs and the list of
> > bugs, I'm going through them now to mark them for the backport request
> > but I think there is some need for a last batch of testing before we
> > finally hit the integration button, and we can take the opportunity to
> > have a final discussion over those patches during the committer's
> > workshop.
> >
> > Before moving on, I wish to thank everyone who was involved in this
> > effort. This was truly an example in collaboration and was amazing. I
> > wish to thank in particular:
> >
> > Andrey Petushkov
> > Sanhong Li
> > Jaroslav Bachorik
> > Marcus Hirt
> > Ekaterina Vergizova
> > Denghui Dong
> > Martin Balao
> > Jiri Vanek
> > Andrew Hughes
> > Aleksey Shipilev
> > Andrew Haley
> >
> > And everyone else who was involved either directly or indirectly I am
> > missing from this list, but has supported the efforts with testing,
> > suggestions and fixes, and that includes of course the original
> > authors for most of those fixes.
> >
> > Also, I wish to thank personally Joe Darcy for his thorough
> > investigation for the CSR, for his feedback and support.
> >
> > This is probably the largest backport attempted so far, so while we
> > had many eyes and spent many hours testing, we know there is still
> > work to do. Merging this code this early in the cycle between releases
> > will give us a chance to iron out existing issues before the April
> > release.
> >
> > Another note, those changes won't enable JFR automatically, the
> > configure flag --enable-jfr will still need to be passed, without this
> > OpenJDK binaries won't have JFR. This is done for compatibility and
> > also as a safety net.
> >
> > Ok, without further ado, here's the webrev and the list of changes as
> > Andrew John Hughes has kindly provided:
> >
> > https://cr.openjdk.java.net/~andrew/openjdk8/jfr/hotspot/
> > https://cr.openjdk.java.net/~andrew/openjdk8/jfr/jdk/
> > https://cr.openjdk.java.net/~andrew/openjdk8/jfr/root/
> >
> > against jdk8u252-b01 and merge changesets:
> >
> > https://cr.openjdk.java.net/~andrew/openjdk8/jfr/hotspot/merge.changeset
> > https://cr.openjdk.java.net/~andrew/openjdk8/jfr/jdk/merge.changeset
> > https://cr.openjdk.java.net/~andrew/openjdk8/jfr/root/merge.changeset
> >
> > Changes:
> >   - S8003209: JFR events for network utilization
> >   - S8041626: Shutdown tracing event
> >   - S8165675: Trace event for thread park has incorrect unit for timeout
> >   - S8195817: JFR.stop should require name of recording
> >   - S8195818: JFR.start should increase autogenerated name by one
> >   - S8195819: Remove recording=x from jcmd JFR.check output
> >   - S8199712: Flight Recorder
> >   - S8202578: Revisit location for class unload events
> >   - S8202835: jfr/event/os/TestSystemProcess.java fails on missing events
> >   - S8203287: Zero fails to build after JDK-8199712 (Flight Recorder)
> >   - S8203346: JFR: Inconsistent signature of jfr_add_string_constant
> >   - S8203664: JFR start failure after AppCDS archive created with JFR
> > StartFlightRecording
> >   - S8203921: JFR thread sampling is missing fixes from JDK-8194552
> >   - S8203929: Limit amount of data for JFR.dump
> >   - S8205516: JFR tool
> >   - S8207392: [PPC64] Implement JFR profiling
> >   - S8207829: FlightRecorderMXBeanImpl is leaking the first classloader
> > which calls it
> >   - S8209960: -Xlog:jfr* doesn't work with the JFR
> >   - S8210024: JFR calls virtual is_Java_thread from ~Thread()
> >   - S8211239: Build fails without JFR: empty JFR events signatures mismatch
> >   - S8212232: Wrong metadata for the configuration of the cutoff for old
> > object sample events
> >   - S8213015: Inconsistent settings between JFR.configure and
> > -XX:FlightRecorderOptions
> >   - S8213421: Line number information for execution samples always 0
> >   - S8213617: JFR should record the PID of the recorded process
> >   - S8213914: [TESTBUG] Several JFR VM events are not covered by tests
> >   - S8213917: [TESTBUG] Shutdown JFR event is not covered by test
> >   - S8213966: The ZGC JFR events should be marked as experimental
> >   - S8214542: JFR: Old Object Sample event slow on a deep heap in debug
> > builds
> >   - S8214750: Unnecessary <p> tags in jfr classes
> >   - S8214896: JFR Tool left files behind
> >   - S8214906: [TESTBUG] jfr/event/sampling/TestNative.java fails with
> > UnsatisfiedLinkError
> >   - S8214925: JFR tool fails to execute
> >   - S8215175: Inconsistencies in JFR event metadata
> >   - S8215237: jdk.jfr.Recording javadoc does not compile
> >   - S8215284: Reduce noise induced by periodic task getFileSize()
> >   - S8215362: JFR GTest JfrTestNetworkUtilization fails
> >   - S8215771: The jfr tool should pretty print reference chains
> >   - S8216064: -XX:StartFlightRecording:settings= doesn't work properly
> >   - S8216486: Possibility of integer overflow in JfrThreadSampler::run()
> >   - S8216559: [JFR] Native libraries not correctly parsed from
> > /proc/self/maps
> >   - S8216578: Remove unused/obsolete method in JFR code
> >   - S8216995: Clean up JFR command line processing
> >   - S8217744: [TESTBUG] JFR TestShutdownEvent fails on some systems due
> > to process surviving SIGINT
> >   - S8217748: [TESTBUG] Exclude TestSig test case from JFR TestShutdownEvent
> >   - S8218935: Make jfr strncpy uses GCC 8.x friendly
> >   - S8223147: JFR Backport
> >   - S8223689: Add JFR Thread Sampling Support
> >   - S8223690: Add JFR BiasedLock Event Support
> >   - S8223691: Add JFR G1 Region Type Change Event Support
> >   - S8223692: Add JFR G1 Heap Summary Event Support
> >   - S8224172: assert(jfr_is_event_enabled(id)) failed: invariant
> >   - S8226779: [TESTBUG] Test JFR API from Java agent
> >   - S8227011: Starting a JFR recording in response to JVMTI VMInit and /
> > or Java agent premain corrupts memory
> >   - S8227605: Kitchensink fails "assert((((klass)->trace_id() &
> > (JfrTraceIdEpoch::leakp_in_use_this_epoch_bit())) != 0)) failed: invariant"
> >   - S8229366: JFR backport allows unchecked writing to memory
> >   - S8229401: Fix JFR code cache test failures
> >   - S8229708: JFR backport code does not initialize
> >   - S8229873: 8229401 broke jdk8u-jfr-incubator
> >   - S8230448: [test] JFRSecurityTestSuite.java is failing on Windows
> >   - S8230707: JFR related tests are failing
> >   - S8230947: TestLookForUntestedEvents.java is failing after JDK-8230707
> >   - S8231995: two jtreg tests failed after 8229366 is fixed
> >   - S8233623: Add classpath exception to copyright in
> > EventHandlerProxyCreator.java file
> >   - S8236002: CSR for JFR backport suggests not leaving out the package-info
> >   - S8236008: Some backup files were accidentally left in the hotspot tree
> >   - S8236074: Missed package-info
> >   - S8236174: Should update javadoc since tags
> >   - S8238076: Fix OpenJDK 7 Bootstrap Broken by JFR Backport
> >
> > 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