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

[David Holmes]

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