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

Mon Feb 24 11:58:22 UTC 2020

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
> 