RFR(XL): 8199712: Flight Recorder

Markus Gronlund markus.gronlund at oracle.com
Thu May 3 18:33:05 UTC 2018 

Hi Karen,

many thanks for taking a look. I have added a few, short and complementary answers inline to Erik's on your first set of questions.

Again, much appreciate you looking into this.

Thanks
Markus

-----Original Message-----
From: Erik Gahlin 
Sent: den 3 maj 2018 17:37
To: Karen Kinnear <karen.kinnear at oracle.com>
Cc: hotspot-jfr-dev at openjdk.java.net; hotspot-dev Source Developers <hotspot-dev at openjdk.java.net>
Subject: Re: RFR(XL): 8199712: Flight Recorder

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.

[MG]
Answer:
Yes I agree, "is_platform_trusted" is already removed; I will change the name of the time source and add a disclaimer (think you wrote some of the original wording):

// Not guaranteed to be synchronized across hardware threads and
// therefore software threads, and can be updated asynchronously
// by software. now() can jump backwards as well as jump forward
// when threads query different cores/sockets.
// Very much not recommended for general use. Caveat emptor.

class FastUnorderedElapsedCounterSource {
 public:
  typedef jlong Type;
  static uint64_t frequency();
  static Type now();
  static double seconds(Type value);
  static uint64_t milliseconds(Type value);
  static uint64_t microseconds(Type value);
  static uint64_t nanoseconds(Type value);
};

> 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

[MG]
Answer:
Yes, it is possible to build the VM without JFR. The current patch has this configure capability:
--enable-jfr=[no/yes]

Although that will change (input from build team), and there will instead be this (updated in next webrev):
--with-jvm-features=-jfr

Listing “–jfr” (note the minus sign) in here will build a VM with no JFR support.

I think we have lost track of what the actual requirements are currently for MinimalVM and if this is (still) the main reason we do this. The image size reduction for excluding JFR at compile time is minimal indeed, but since we already had the conditional model from the open/closed split, we brought that over pretty much as-is.

Please see my earlier mail to Coleen about the headers and include guards,  http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-May/032027.htm 

> 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.h
> pp
> + //
> + // 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?

[MG]
Answer:
This is a new capability that will allow for better handing inside and around the vicinity of critical sections (such as ObjectMonitor::Enter()):

This code does not store the stacktrace in the _owner field however, instead:

The additional functionality will ensure that the thread has enough space (thread local) to accomplish a write of a JavaMonitorEvent and will also capture and save the java stacktrace  _before_ entering wait. This is to ensure that we need not do these operations at the point of returning from the wait, which implies we are now the holder of the monitor. This relative simple solution allows us to do any "slower path" work pre-emptively, instead of when the thread is the exclusive owner of some precious resource. Note that the flush is conditional, so it is only done iff the event will not fit.
This solution is generic and might be put to good effect at other sites as well. There is some small follow-up work here though in that we currently don't have good size estimation for events that use strings.

> 5. jfrMacros.hpp
> I don't see any users of JFR_FLUSH?
>
Markus will comment 4 and 5.

[MG]
Answer:
See previous answer, this primitive is part of the extended capability. Might not yet be used at sites.

> 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/03135
>>>> 9.html
>>>>
>>>> Thanks
>>>> Erik and Markus
>>>
>>>
>>> --
>>> Dmitry Samersoff
>>> http://devnull.samersoff.net
>>> * There will come soft rains ...
>>>
>>
>

More information about the hotspot-jfr-dev mailing list