RFR(XL): 8199712: Flight Recorder

Thu May 3 15:36:38 UTC 2018

On 2018-05-02 00:08, Karen Kinnear wrote:
> 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”?
>
I think the idea is that tags should be kept simple, so they can be 
reused by different subsystems, and then you combine them when you log 
things, i.e. jfr+setting

> 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
>
I will update the CSR and the code, but shouldn't we use Expired, since 
the feature is removed.

(If somebody actually built something on top of -XX:EnableTracing you 
want it to fail fast. Starting the JVM even though the feature is 
removed is hiding problems, which may make things worse at a later stage.)

> 3. CSR question - did jcmd change at all?
Not really. We did a small code change that impacts 
-XX:StartFlightRecording and JFR.start.  If you specify 
-XX:StartFlightRecording=filename=dump.jfr a recording will be dumped on 
exit. This is the only reasonable behavior. You wouldn't specify a 
filename unless you wanted to dump it. In previous CSRs we have not 
specified behavior/heuristics of flags that are unspecified. If a user 
specifies 
-XX:StartFlightRecording=filename=dump.jfr,dumponexit=true/false we 
still honor that.

> 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.
>
Markus will comment fast time support.

> 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.
>
See separate e-mail
> 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.
>
We have added a RFE for this:

https://bugs.openjdk.java.net/browse/JDK-8202579
> 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.
>
Will fix.

> 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?
>
Markus will comment 4 and 5.

> 6. vmStructs.cpp removed all the VM_XXX_TRACE which was conditional.
> Did SA never use it? Or will that need updating?
>
The SA support has not worked since JDK 9. Bugs have been filed, but 
they have not been fixed.

The support was added by sustaining so it was possible to extract a .jfr 
file from a core dump. With JDK 9, a recording is written if a crash 
occurs, so the functionality is not needed anymore. Even if somebody 
would like the functionality back, for whatever reason, most of 
declarations are wrong anyway. I don't think we should bring in code 
that is broken into OpenJDK.

SA is also a nightmare to maintain. Let's keep it out.

> 7. Assuming jdk.management.jfr and jdk.jfr changes were just 
> name/include locations/copyrights
> but nothing substantial - I did a quick skim
We have only changed copyright headers and spelling errors in the jdk 
modules.

Thanks
Erik

>
> thanks,
> Karen
>
>> On Apr 28, 2018, at 4:17 AM, Erik Gahlin <erik.gahlin at oracle.com 
>> <mailto: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 
>>> <mailto: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 ...
>>>
>>
>