RFR(XL): 8199712: Flight Recorder

Fri May 4 14:17:07 UTC 2018

Markus,

Thanks for the changes and taking time to explain things.

> On May 3, 2018, at 2:33 PM, Markus Gronlund <markus.gronlund at oracle.com> wrote:
> 
> 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 <mailto:karen.kinnear at oracle.com>>
> Cc: hotspot-jfr-dev at openjdk.java.net <mailto:hotspot-jfr-dev at openjdk.java.net>; hotspot-dev Source Developers <hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>>
> Subject: Re: RFR(XL): 8199712: Flight Recorder
> ...
> 
>> 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):
Thank you very much for the changes :-)
> 
> // 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);
> };
> 
>> 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.
I believe we still have reasons to build a MinimalVM. I don’t know the JFR size impact, so it may be moot. Thank you for bringing this over
and yes this is something we will want to explore more down the road.
> 
> 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 <http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-May/032027.htm> 
I saw that and liked the approach.
> 
>> 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 
Many thanks.
> 
>> 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.
Thank you for the explanation. Totally appreciate the goal and the approach. 
> 
>> 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.
> 
thanks,
Karen
>> 
>>> On Apr 28, 2018, at 4:17 AM, Erik Gahlin <erik.gahlin at oracle.com 
>>> <mailto: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> 
>>>> <mailto: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 ...