Thank you very much, Serguei!
- Jini.
On 11/30/2017 2:16 PM, serguei.spit...@oracle.com wrote:
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, serguei.spit...@oracle.com 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.
