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

List:       openjdk-hotspot-runtime-dev
Subject:    Request for review: 7132690 InstanceKlass:_reference_type should be u1 type
From:       jiangli.zhou () oracle ! com (Jiangli Zhou)
Date:       2012-01-26 1:18:02
Message-ID: 4F20A9CA.3050207 () oracle ! com
[Download RAW message or body]

Thanks, David.

Jiangli

On 01/25/2012 05:09 PM, David Holmes wrote:
> Looks good to me.
>
> David
>
> On 26/01/2012 4:32 AM, Jiangli Zhou wrote:
>> Hi Vitaly and David,
>>
>> I've updated the webrev to do a == check via the cast.
>>
>> http://cr.openjdk.java.net/~jiangli/7132690/webrev.02/
>>
>> Thanks,
>>
>> Jiangli
>>
>> On 01/24/2012 11:44 PM, David Holmes wrote:
>>> On 25/01/2012 2:41 PM, Vitaly Davidovich wrote:
>>>> Hi Jiangli,
>>>>
>>>> I think doing the comparison via the cast and == check to make sure it
>>>> "roundtrips" is better than explicit range check because this way you
>>>> don't have to change the assert if you change the width of the type
>>>> (e.g. using u2 instead of u1 would require updating the range). 
>>>> Plus as
>>>> Dean mentioned, it looks like the cast version is already used in the
>>>> codebase.
>>>
>>> Plus the range check assumes that the enum will never have negative
>>> values (not likely here but why preclude it).
>>>
>>> David
>>>
>>>> Cheers
>>>>
>>>> Sent from my phone
>>>>
>>>> On Jan 24, 2012 10:19 PM, "Jiangli Zhou" <jiangli.zhou at oracle.com
>>>> <mailto:jiangli.zhou at oracle.com>> wrote:
>>>>
>>>> __
>>>> Hi Vitaly,
>>>>
>>>> Thanks for catching it! A range check seems to be more explicit in
>>>> this case:
>>>>
>>>> http://cr.openjdk.java.net/~jiangli/7132690/webrev.02/
>>>>
>>>> Thanks,
>>>>
>>>> Jiangli
>>>>
>>>> On 01/24/2012 04:17 PM, Vitaly Davidovich wrote:
>>>>>
>>>>> I think you'll be fully covered (including negative values) by
>>>>> doing the following instead of hard coding max value in the assert:
>>>>>
>>>>> assert(t == (u1)t, "doesn't fit")
>>>>>
>>>>> Sent from my phone
>>>>>
>>>>> On Jan 24, 2012 6:33 PM, "Jiangli Zhou" <jiangli.zhou at oracle.com
>>>>> <mailto:jiangli.zhou at oracle.com>> wrote:
>>>>>
>>>>> The updated webrev is :
>>>>> http://cr.openjdk.java.net/~jiangli/7132690/webrev.01/
>>>>> <http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.01/>
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>> On 01/24/2012 02:54 PM, Jiangli Zhou wrote:
>>>>>> Hi Vitaly,
>>>>>>
>>>>>> An assert in the setter probably is a good idea. I should
>>>>>> have added it when making the change. Thanks for the comments!
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>>
>>>>>> On 01/24/2012 02:13 PM, Vitaly Davidovich wrote:
>>>>>>>
>>>>>>> Hi Jiangli,
>>>>>>>
>>>>>>> This is probably overly paranoid so feel free to ignore, but
>>>>>>> should the setter in InstanceKlass assert that the passed in
>>>>>>> ReferenceType fits into a u1 instead of silently narrowing
>>>>>>> it? Or change the setter to take a u1 and make caller do the
>>>>>>> cast? This would prevent someone defining another member of
>>>>>>> the enum with an explicit value that doesn't fit into u1.
>>>>>>> Like I said, paranoia ... :)
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Sent from my phone
>>>>>>>
>>>>>>> On Jan 24, 2012 3:06 PM, "Jiangli Zhou"
>>>>>>> <jiangli.zhou at oracle.com <mailto:jiangli.zhou at oracle.com>>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the change for 7132690:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jiangli/7132690/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.00/>
>>>>>>>
>>>>>>> It changes InstanceKlass::_reference_type from
>>>>>>> ReferenceType to u1. The ReferenceType is defined as an
>>>>>>> enum with 6 values
>>>>>>> (src/share/vm/utilities/globalDefinitions.hpp). The
>>>>>>> change saves 4-byte for each class.
>>>>>>>
>>>>>>> Tested with runThese and ute vm.quick.testlist. Ran jprt.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Jiangli
>>>>>>>
>>>>>>
>>>>>
>>>>
>>


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