Request for review: 7132690 InstanceKlass:_reference_type should be u1 type

[Vitaly Davidovich]

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.

Cheers

Sent from my phone
On Jan 24, 2012 10:19 PM, "Jiangli Zhou" <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> wrote:
>
>>  The updated webrev is :
>> http://cr.openjdk.java.net/~jiangli/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> wrote:
>>
>>> Hi,
>>>
>>> Please review the change for 7132690:
>>>
>>>  http://cr.openjdk.java.net/~jiangli/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
>>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120124/391ea0b1/attachment-0001.html