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

[Jini George]

Thank you very much, Serguei!

- Jini.

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