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

[Jini George]

Thanks much, David!

- Jini.

On 11/30/2017 1:17 PM, 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 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.
>>>>
>>>