RFR JMC-6075: Avoid conversion for RECORDING_SETTING event from JDK9/10 in SettingsTransformer

[Jayathirth D V]

Hi Marcus,

Sharath has approved latest webrev : http://cr.openjdk.java.net/~jdv/6075/webrev.01/ 
Please review the latest change.

Thanks,
Jay

-----Original Message-----
From: Sharath Ballal 
Sent: Saturday, July 21, 2018 9:08 PM
To: Jayathirth D V; jmc-dev at openjdk.java.net
Subject: RE: RFR JMC-6075: Avoid conversion for RECORDING_SETTING event from JDK9/10 in SettingsTransformer

Looks Good Jay.


Thanks,
Sharath


-----Original Message-----
From: Jayathirth D V 
Sent: Saturday, July 21, 2018 2:20 PM
To: Sharath Ballal; jmc-dev at openjdk.java.net
Subject: RE: RFR JMC-6075: Avoid conversion for RECORDING_SETTING event from JDK9/10 in SettingsTransformer

Hi Sharath,

Thanks for your inputs.

The needTransform() logic that was present was not differentiating between JDK7/8 & JDK 9/10 events. It has the logic to differentiate between JDK7/8/9/10 & JDK 11 events.
public static boolean needTransform(String typeId) {
                 if (typeId.startsWith(PREFIX)) {
                         return false;
                 }
                 return typeId.startsWith(EVENT_ID_ROOT) || typeId.startsWith(PREFIX_9_10);
         }

So needTransform variable previously present for validating SettingTransformer object was not differentiating SettingsTransformer created between JDK7/8 & JDK9/10.
Even when we use needTransform variable, If we get JDK 7/8 events and st.isValidV1() returns true we will still use the invalid SettingsTransformer object.

That said, we should have tighter checks for validating SettingsTransformer object differently between JDK 7/8 & JDK 9/10. So I have added new checks before we return SettingsTransformer object.

Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6075/webrev.01/ 

No unit test failures with latest code. There are no differences in Automated analysis results too.

Thanks,
Jay

-----Original Message-----
From: Sharath Ballal 
Sent: Friday, July 20, 2018 10:09 PM
To: Jayathirth D V; jmc-dev at openjdk.java.net
Subject: RE: RFR JMC-6075: Avoid conversion for RECORDING_SETTING event from JDK9/10 in SettingsTransformer

Hi Jay,

SettingsTransformer.java

-                                        if (st.isValid() || (needsTransform && st.isValidV1())) {
+                                        if (st.isValid() || st.isValidV1()) {

You should not remove the ' needsTransform' here.  We are using it to distinguish between JDK 7/8 and JDK 9/10.  Else for 7/8 if st.isValidV1() returns true the condition will go through even though it should not.


Thanks,
Sharath


-----Original Message-----
From: Jayathirth D V 
Sent: Friday, July 20, 2018 1:35 PM
To: jmc-dev at openjdk.java.net
Subject: RFR JMC-6075: Avoid conversion for RECORDING_SETTING event from JDK9/10 in SettingsTransformer

Hello All,

 

Please review the following fix in JMC 7 :

 

Bug : https://bugs.openjdk.java.net/browse/JMC-6075 

Webrev : http://cr.openjdk.java.net/~jdv/6075/webrev.00/

 

Issue: In SettingTransformer and SyntheticAttributeExtension, whenever we have RECORDING_SETTING from JDK9/10 we translate these events to JDK11 type.

 

Solution: We have static variables which we use for Pre JDK9 events in JdkTypeIDsPreJdk11. For JDK9/10 RECORDING_SETTING event type also we can maintain a static variable in JdkTypeIDsPreJdk11 and use it wherever applicable. This will avoid conversion of JDK9/10 RECORDING_SETTING event to JDK11 type. I have removed usage of JdkTypeIDsPreJdk11.needTransform() because we will be creating SettingTransform object only when have Pre-JDK 11 RECORDING_SETTING event type. Also there is some cleanup done in  SettingTransformer.

 

I have ran unit tests and there are no failures. Also used JDK 7/8/9/10/11 recordings and I don't see any difference in Automated-analysis results before and after change.

 

Thanks,

Jay