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

[Vitaly Davidovich]

looks good although I'm not a reviewer :)

Vitaly

Sent from my phone
On Jan 25, 2012 1:33 PM, "Jiangli Zhou" <jiangli.zhou at oracle.com> 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/<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 <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/<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 <jiangli.zhou at oracle.com>>> wrote:
>>>>
>>>>        The updated webrev is :
>>>>        http://cr.openjdk.java.net/~**jiangli/7132690/webrev.01/<http://cr.openjdk.java.net/~jiangli/7132690/webrev.01/>
>>>> <http://cr.openjdk.java.net/%**7Ejiangli/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<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/~jiangli/7132690/webrev.00/>
>>>>>> <http://cr.openjdk.java.net/%**7Ejiangli/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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120125/25f7ebd8/attachment.html