RFR(XL): 8199712: Flight Recorder

Fri May 4 14:10:10 UTC 2018

Erik,

Thanks for the detailed explanations and responding to all points raised :-)

> On May 3, 2018, at 11:36 AM, Erik Gahlin <erik.gahlin at oracle.com> wrote:
> 
> 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 
Thanks for the explanation - I had missed that.
> 
>> 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.)
Let me clarify - the approach we take is to remove the functionality but leave the flags for one release in order to
give people time to update their scripts. 
If you read arguments.cpp it describes the deprecation process.
You did a 1-step - complete removal - i.e Expired.
What I am asking is that you change this to a 2-step - first go to Obsolete - i.e. leave the flags in so their scripts won’t break,
but will print a warning, and do nothing when the flags are set.
We have evolved this to be more customer friendly.
> 
>> 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. 
Thank you for the explanation. All set.
> 
>> If it did I would expect that to also be covered in the CSR,
>> but I think it did not.
>> 
thanks,
Karen
...
> 
> 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 < <mailto:dms at samersoff.net>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 <https://bugs.openjdk.java.net/browse/JDK-8193393>
>>>>> 
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~egahlin/8199712.0/ <http://cr.openjdk.java.net/~egahlin/8199712.0/>
>>>>> 
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8199712 <https://bugs.openjdk.java.net/browse/JDK-8199712>
>>>>> 
>>>>> [1]
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-April/031359.html <http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-April/031359.html>
>>>>> 
>>>>> Thanks
>>>>> Erik and Markus
>>>> 
>>>> 
>>>> -- 
>>>> Dmitry Samersoff
>>>> http://devnull.samersoff.net <http://devnull.samersoff.net/>
>>>> * There will come soft rains ...
>>>> 
>>> 
>> 
>