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

[Jiangli Zhou]

Thanks for the review in any case! :)

Jiangli

On 01/25/2012 10:34 AM, Vitaly Davidovich wrote:
>
> 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 
> <mailto: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/%7Ejiangli/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>
>             <mailto: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/
>             <http://cr.openjdk.java.net/%7Ejiangli/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>
>                 <mailto: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/>
>                 <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>
>                         <mailto: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/>
>                         <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/472ead1a/attachment-0001.html