Re: RFR: JDK-8191324: SA cleanup -- part 2

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8191324: SA cleanup -- part 2

serguei.spitsyn@oracle.com
Hi Jini,

It looks good to me.

A couple of questions on the:
  http://cr.openjdk.java.net/~jgeorge/8191324/webrev.00/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/PerfDataEntry.java.udiff.html

+    private static int U_None;
+    private static int U_Bytes;
+    private static int U_Ticks;
+    private static int U_Events;
+    private static int U_String;
+    private static int U_Hertz;
+
     . . .
+
+        // PerfData Units enum
+        U_None = db.lookupIntConstant("PerfData::U_None");
+        U_Bytes = db.lookupIntConstant("PerfData::U_Bytes");
+        U_Ticks = db.lookupIntConstant("PerfData::U_Ticks");
+        U_Events = db.lookupIntConstant("PerfData::U_Events");
+        U_String = db.lookupIntConstant("PerfData::U_String");
+        U_Hertz = db.lookupIntConstant("PerfData::U_Hertz");


Before your fix these private static fields were defined in the interface which I like:
-    public interface PerfDataUnits {
-        public static final int U_None   = 1;
-        public static final int U_Bytes  = 2;
-        public static final int U_Ticks  = 3;
-        public static final int U_Events = 4;
-        public static final int U_String = 5;
-        public static final int U_Hertz  = 6;
-    }

I think, it'd still make sense to keep them in an interface or a small class,
so they are not separate constants but a part of a common outer type.

-    public interface PerfDataVariability {
-        public static final int V_Constant  = 1;
-        public static final int V_Monotonic = 2;
-        public static final int V_Variable  = 3;
-    }

I wonder it these constants can still be useful as the following method returns
one of them as a result which may need to be checked for an exact value.


Thanks,
Serguei



On 11/21/17 02:34, Jini George wrote:
Hello,

Here's requesting reviews for some SA code cleanup.

ID: https://bugs.openjdk.java.net/browse/JDK-8191324
Webrev: http://cr.openjdk.java.net/~jgeorge/8191324/webrev.00/index.html

The changes here are primarily to:

1. Remove unused IA64 SA code.
2. Make changes to avoid error-prone redefinition of hotspot constants in SA Java code. Instead read it in through vmStructs and db.lookupIntConstant().
3. Make variable name changes in SA to align with the hotspot names.

The changes are straightforward.

Thanks much,
Jini.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8191324: SA cleanup -- part 2

serguei.spitsyn@oracle.com
Hi Jini,

It looks good to me.
Thank you for the update!

A minor tip:
 It'd match the Hotspot naming better if the PerfDataUnits is replaced with PerfData.

But I leave it up to you.

Thanks,
Serguei


On 11/29/17 23:47, David Holmes wrote:
Hi Jini,

On 29/11/2017 9:51 PM, Jini George wrote:
Thank you very much for the review, Serguei. Continuing to keep these constants in an interface would mean that they would need to have the 'final' qualifier. And that would mean that we would not be able to populate the values of these constants at runtime by reading those in from vmStructs using db.lookupIntConstant(). I have instead made these as inner classes in this new webrev:

http://cr.openjdk.java.net/~jgeorge/8191324/webrev.01/

I had a look at this and it all seems fine. Good to see the ia64 code gone!

As David had pointed out wrt the review of 8190837: BasicType and BasicTypeSize should refer to HotSpot values, I realize that removal of the 'final' qualifier could cause parfait warnings, but since we would need to do that for the rest of the constants in the file also, I would prefer taking it up as a separate exercise.

The fact it is a private inner class will probably be enough to stop parfait from flagging the non-final static fields. You should also be able to declare them private (or at least package-access) rather than public, which further removes them from the kind of construct parfait is looking for.

Thanks,
David
-----

I had removed the PerfDataVariability constants since these were not used, and we are trying to remove unused code in SA to reduce the probability of bugs creeping in. Guess we can always put it back if we start checking the values against these constants. Let me know if you don't agree with this.

Thanks,
Jini.



On 11/28/2017 12:38 PM, [hidden email] wrote:
Hi Jini,

It looks good to me.

A couple of questions on the:
http://cr.openjdk.java.net/~jgeorge/8191324/webrev.00/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/PerfDataEntry.java.udiff.html

+ private static int U_None;
+ private static int U_Bytes;
+ private static int U_Ticks;
+ private static int U_Events;
+ private static int U_String;
+ private static int U_Hertz;
+. . . +
+ // PerfData Units enum
+ U_None = db.lookupIntConstant("PerfData::U_None");
+ U_Bytes = db.lookupIntConstant("PerfData::U_Bytes");
+ U_Ticks = db.lookupIntConstant("PerfData::U_Ticks");
+ U_Events = db.lookupIntConstant("PerfData::U_Events");
+ U_String = db.lookupIntConstant("PerfData::U_String");
+ U_Hertz = db.lookupIntConstant("PerfData::U_Hertz");Before your fix these private static fields were defined in the interface which I like:

- public interface PerfDataUnits {
- public static final int U_None = 1;
- public static final int U_Bytes = 2;
- public static final int U_Ticks = 3;
- public static final int U_Events = 4;
- public static final int U_String = 5;
- public static final int U_Hertz = 6;
- }


I think, it'd still make sense to keep them in an interface or a small class,
so they are not separate constants but a part of a common outer type.

- public interface PerfDataVariability {
- public static final int V_Constant = 1;
- public static final int V_Monotonic = 2;
- public static final int V_Variable = 3;
- } I wonder it these constants can still be useful as the following method returns one of them as a result which may need to be checked for an exact value.Thanks, Serguei




On 11/21/17 02:34, Jini George wrote:
Hello,

Here's requesting reviews for some SA code cleanup.

ID: https://bugs.openjdk.java.net/browse/JDK-8191324
Webrev: http://cr.openjdk.java.net/~jgeorge/8191324/webrev.00/index.html

The changes here are primarily to:

1. Remove unused IA64 SA code.
2. Make changes to avoid error-prone redefinition of hotspot constants in SA Java code. Instead read it in through vmStructs and db.lookupIntConstant().
3. Make variable name changes in SA to align with the hotspot names.

The changes are straightforward.

Thanks much,
Jini.