Request for review 8000725: NPG: method_holder() and pool_holder() and _pool_holder field should be InstanceKlass

[David Holmes]

On 29/10/2012 10:38 PM, harold seigel wrote:
> Hi David,
>
> We will have a webrev for review later this week that will remove the
> Klass::cast() function and all its uses (see bug 8001471). We thought it
> would be easier to just remove the unnecessary InstanceKlass::cast()
> calls in this webrev (along with a few Klass::cast() calls).

Can you comment on the loss of asserts and the failure modes those 
asserts were covering?

David

> Thanks for reviewing it!
>
> Harold
>
> On 10/27/2012 4:02 AM, David Holmes wrote:
>> Hi Harold,
>>
>> On 27/10/2012 4:11 AM, harold seigel wrote:
>>> Please review the following changes.
>>>
>>> Summary: Change the function return type of method_holder(),
>>> field_holder(), and pool_holder() to InstanceKlass. Also, change the
>>> type of field _pool_holder to InstanceKlass. These changes enabled the
>>> removal of lots of InstanceKlass::cast() calls from the code.
>>
>> There have also been a lot of removals of Klass::cast even though the
>> previous return types were Klass*. So looking at Klass::cast I see
>>
>> // Casting
>> static Klass* cast(Klass* k) {
>> assert(k->is_klass(), "cast to Klass");
>> return k;
>> }
>>
>> which begs the question: how can k->is_klass() not be true? And have
>> we lost anything by getting rid of these "casts"? There are additional
>> checks in instanceKlass::cast too
>>
>> // Casting from Klass*
>> static InstanceKlass* cast(Klass* k) {
>> assert(k->is_klass(), "must be");
>> assert(k->oop_is_instance(), "cast to InstanceKlass");
>> return (InstanceKlass*) k;
>> }
>>
>> Is the need for the asserts obsolete in a NPG world?
>>
>> That aside in vframe.cpp:
>>
>> + InstanceKlass* k = m->method_holder();
>> tty->print_cr("frame( sp=" INTPTR_FORMAT ", unextended_sp="
>> INTPTR_FORMAT ", fp=" INTPTR_FORMAT ", pc=" INTPTR_FORMAT ")",
>> _fr.sp(), _fr.unextended_sp(), _fr.fp(), _fr.pc());
>> tty->print("%s.%s", Klass::cast(k)->internal_name(),
>> m->name()->as_C_string());
>>
>> there is a Klass::cast that can be removed.
>>
>> Thanks,
>> David
>> -----
>>
>>>
>>> This webrev has a lot of files but most of the changes are small.
>>>
>>> Open webrev at http://cr.openjdk.java.net/~coleenp/bug_8000725
>>> <http://cr.openjdk.java.net/%7Ecoleenp/bug_8000725>
>>>
>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8000725
>>>
>>> The changes were tested with JPRT, JCK, and other tests including ones
>>> for sa-jdi.
>>>
>>> Thanks, Harold