|
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.
|
