RFR(XL): 8199712: Flight Recorder

Tue May 1 22:08:04 UTC 2018

Lots of great work here. Review part 1 below.

I will not be reviewing build files or test files - I believe you have other coverage for those. I did this quickly,
so if any of my comments don’t make sense after detailed study - just let me know.

I owe you a review of the share/jfr files still.

A couple of issues that I think should be addressed before checking in the code:

1. New logTag.hpp log tags
One is called “setting” -> could you make this less general please? like jfrsetting?
Or this a subcategory so we already know we are under “jfr”?

Note: I would expect that log tags would be added to the CSR since that is a supported interface

2. Code removed two command line flags: EnableTracing and UseLockedTracing
 with product flags, the normal approach is to deprecate
 given they now do nothing, I would strongly recommend that you Obsolete before removing
i.e. accept the flags, but do not do anything for 1 release, to allow script migration. Expire the flags
in the next release. So test that you get a warning if you use them for JDK 11.

Note: CSR says EnableTracing flag is removed -> please update CSR for both to be made obsolete

3. CSR question - did jcmd change at all? If it did I would expect that to also be covered in the CSR,
but I think it did not.

A couple of issues that should be addressed before JDK11 ships

1. FastTime naming - we need to not present an attractive nuisance here.
Please rename to clarify at least:
  is_platform_trusted_for_fast_time() -> “platform_provides_fast_unordered_time” or something equally clear about risk in the name
     - there is a local with a similar need to change name
  FastTime* APIs: e.g. FastTimeElapsedCounterSource -> please include Unordered in the name
  For folks new to this issue - the rdtsc (read time stamp counter) instruction can return different times across different sockets,
  so time can go backwards, so we do not use this for any vm or java times, just for diagnostic timestamps 
  and we highly recommend that folks modifying the jvm do not start using it accidentally.

2. Docker container compatibility
Can you please check with Bob Vandette e.g. for cgroup handling - the new os_perf_linux.cpp may have some overlap/need
modifications simliar to what he did with os_linux.cpp - e.g. for number of cpus, sockets, etc.
And please test with a container - Misha can give guidance here.

Other comments:

1. #INCLUDE_JFR
Did you build with #INCLUDE_JFR not defined?
Is the theory that we still may want a minimalVM without JFR?
The model is not clear to me in terms of when you include files or generate events
e.g. c1_GraphBuilder.cpp, systemDictionary.cpp, synchronizer.cpp, …
Could this please be on your future clean-up list?
p.s. I do like the extraction of the post_xxx_event logic

2. Duplication
Is there any duplication in the VM_Version and VM_Version_ext handling?
And are you covering older CPUs we no longer need to cover? Vladimir K would know the precise list.

3.  objectCountEventSender.hpp: comment to clean up:
+  // The following two functions have the exact same signature as
+  // hotspot/src/share/vm/gc_implementation/shared/objectCountEventSender.hpp
+  //
+  // This file will replace the open file if a closed build is performed.
+  // These function signatures can therefore not be changed if the open
+  // signatures aren't changed as well.

4. ObjectMonitor::enter
 Why was this event modified differently from others - i.e. it stores
 the stack trace in the _owner field? And is there a reason we flush here?

5. jfrMacros.hpp
I don't see any users of JFR_FLUSH?

6. vmStructs.cpp removed all the VM_XXX_TRACE which was conditional.
Did SA never use it? Or will that need updating?

7. Assuming jdk.management.jfr and jdk.jfr changes were just name/include locations/copyrights
but nothing substantial - I did a quick skim

thanks,
Karen

> On Apr 28, 2018, at 4:17 AM, Erik Gahlin <erik.gahlin at oracle.com> wrote:
> 
> Yes, If we don’t include JFR, we should get the stubs
> 
> Will fix.
> 
> Erik
> 
>> On 28 Apr 2018, at 09:59, Dmitry Samersoff <dms at samersoff.net> wrote:
>> 
>> Erik,
>> 
>> globals.cpp:1051 these changes seems to be unnecessary.
>> 
>> -Dmitry
>> 
>> On 25.04.2018 14:06, Erik Gahlin wrote:
>>> Greetings,
>>> 
>>> Could I have a review of 8199712: Flight Recorder
>>> 
>>> As mentioned in the preview [1] the tracing backend has been removed.
>>> Event metadata has been consolidated into a single XML file and event
>>> classes are now generated by GenerateJfrFiles.java.
>>> 
>>> Tests have been run on Linux-x64, Windows-x64 and MaxOSX-x64.
>>> 
>>> For details about the feature, see the JEP:
>>> https://bugs.openjdk.java.net/browse/JDK-8193393
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~egahlin/8199712.0/
>>> 
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8199712
>>> 
>>> [1]
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-April/031359.html
>>> 
>>> Thanks
>>> Erik and Markus
>> 
>> 
>> -- 
>> Dmitry Samersoff
>> http://devnull.samersoff.net
>> * There will come soft rains ...
>> 
> 