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

Jini George jini.george at oracle.com
Wed Nov 29 11:51:05 UTC 2017


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/

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.

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


More information about the hotspot-runtime-dev mailing list