RFR: JMC-5895 - JFR in JDK 11 changed event paths from 'com.oracle.jdk' to 'jdk', and type paths from 'com.oracle.jfr.types' to 'jdk.types'

[Marcus Hirt]

Good points Jay! To make the difference between v1 and v2 more maintainable and clear, we should probably have v1 and v2 being their own parsers with (plenty of) shared infrastructure. That said, since we need to get an EA out soon, I think that can wait. 

I think it may be a good idea to have a Jdk9TypeIDs.java, at least for a subset of the type IDs (like RECORDING_SETTING), but since no one should be using it, except our internal machinery, it should be package visible only. 

I think we can keep createStructReaderV2, as a first step in making a clean separation between v1 and v2, but I am not religious about it.

Kind regards,
Marcus (still on vacation, but back mid next week)

> On 14 Jul 2018, at 21:44, Jayathirth D V <jayathirth.d.v at oracle.com> wrote:
> 
> Hi Sharath,
> 
> Please find my observations:
> 
> After webrev.02 , For each event in the code to check whether we have Event from JDK 9 or 10 we are using JdkTypeIDsPreJdk11.needtransform() function. This might make automated analysis take more time. We can completely remove the usage of this verification if we maintain JDK 9/10 Event types in a file like JdkTypeIDs.java. We can have new file like Jdk9TypeIDs.java with events starting from "com.oracle.jdk.".
> 
> After doing above thing we can make following changes in other files:
> 
> In SettingsTransformer.java:
>    In wrapSinkFactory.create() we can use:
>    if (JdkTypeIDsPreJdk11.RECORDING_SETTING.equals(identifier) ||
>         Jdk9TypeIDs.RECORDING_SETTING.equals(identifier)) {
> 
>    Both these conditions will take care of whether event is from JDK 7/8 & 9/10. We can remove needtransform() check while validating created SettingsTransformer object.
> 
>    In SettingsTransformer() constructor, for Jdk 9/10 events looks like we are not able to find any field which matches END_TIME. So we are creating EventSink with all the data structures. 
>    In SettingsTransformer.addEvent() we can remove the check for JdkTypeIDsPreJdk11.needTransform() because we are creating SettingsTransformer only when we know we have event types from JDK 7/8/9/10.
> 
> In SyntheticAttributeExtension.java:
>    If we have Jdk9TypeIDs.java there is no need to explicitly call JdkTypeIDsPreJdk11.translate().  Also here we can use single if block like:
>        public String getValueInterpretation(String eventTypeId, String fieldId) {
>        if (REC_SETTING_EVENT_ID_ATTRIBUTE.getIdentifier().equals(fieldId)
>                && (JdkTypeIDsPreJdk11.RECORDING_SETTING.equals(eventTypeId) ||
>                    Jdk9TypeIDs.RECORDING_SETTING.equals(eventTypeId) ||
>                    JdkTypeIDs.RECORDING_SETTING.equals(eventTypeId) {
>            return JfrInternalConstants.TYPE_IDENTIFIER_VALUE_INTERPRETATION;
>        }
>        return null;
>    }
> 
> In JdkTypeIDsPreJdk11.java:
>    With above changes we can remove needtransform() function.
> 
> Generic change in TypeManager.java:
>    Looks like there is no need to create new function createStructReaderV2(). We can use createStructReaderV1() by adding new "switch identifiers" as most of them are same between createStructReaderV1() & createStructReaderV2 ().
> 
> Since I am new to JMC project I wanted to verify things before mentioning my observations. So I made above changes locally and was able to build and run unit tests without failure:)
> 
> Thanks,
> Jay
> 
> -----Original Message-----
> From: Sharath Ballal 
> Sent: Saturday, July 14, 2018 10:32 AM
> To: jmc-dev at openjdk.java.net
> Subject: RE: RFR: JMC-5895 - JFR in JDK 11 changed event paths from 'com.oracle.jdk' to 'jdk', and type paths from 'com.oracle.jfr.types' to 'jdk.types'
> 
> The fix has undergone changes from last time as 'mvn verify' was failing.  The code changes now include: 
> 
> 1.      Changes to the V1 parser to use the SettingsTransformer for JDK 9 & 10 events.
> 
> 2.      Changes to some of the tests and their baseline files.
> 
> 
> 
> Testing done:
> 
> mvn verify passes.
> 
> JDK 7/8/9/10 - Recording, loading and automated analysis works.  Verified that  scores of the automated analysis doesn't change from earlier.
> 
> JDK 11 - Recording, loading and automated analysis works.  
> 
> 
> 
> Latest webrev at : http://cr.openjdk.java.net/~sballal/JMC-5895/webrev.03/ 
> 
> Kindly review the fix.
> 
> 
> 
> Thanks,
> 
> Sharath
> 
> 
> 
> 
> 
> From: Sharath Ballal [mailto:sharath.ballal at oracle.com] 
> Sent: Friday, June 08, 2018 3:03 PM
> To: 'jmc-dev at openjdk.java.net'
> Subject: RFR: JMC-5895 - JFR in JDK 11 changed event paths from 'com.oracle.jdk' to 'jdk', and type paths from 'com.oracle.jfr.types' to 'jdk.types'
> 
> 
> 
> Hello,
> 
> 
> 
> Pls review fix for the following P1 issue:
> 
> 
> 
> ID: https://bugs.openjdk.java.net/browse/JMC-5895 
> 
> Webrev: http://cr.openjdk.java.net/~sballal/JMC-5895/webrev.00/ 
> 
> 
> 
> Thanks,
> 
> Sharath
> 
> 
> 
>