[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8191324: SA cleanup -- part 2
From:       Jini George <jini.george () oracle ! com>
Date:       2017-11-30 10:22:15
Message-ID: 74835c6f-d780-eee1-6c49-8850bb6563bb () oracle ! com
[Download RAW message or body]

Thank you very much, Serguei!

- Jini.

On 11/30/2017 2:16 PM, serguei.spitsyn@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.spitsyn@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.
> > > > > 
> > > > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]