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

[David Holmes]

Hi Coleen,

Thanks for explaining why the "fence" is no longer needed. :)

David

On 6/11/2012 2:53 AM, Coleen Phillimore wrote:
>
> The reason that we had InstanceKlass::cast()s all over the place is
> because with Permgen, almost everything was passed around as an "oop".
> The type klassOop was inherited from oop. We needed these asserts
> because things, like during GC and in this other JNI bug you reference,
> would cast down to oop and back up to klassOop frequently. Since these
> are now distinct C++ types, this casting should be completely gone in
> the repository. If you see any casts between "oop" and "klassOop" it is
> a bug and should be fixed. There is one case in CMS we do something like
> this but it's a special case and doesn't escape this context.
>
> Because we are using C++ types now for metadata, the compiler can
> statically check the code. Of course, you can write code like Klass* k =
> (Klass*)5; but having Klass::cast(k) isn't going to be useful to see if
> k->is_Klass(). Having types check that they are really the type declared
> would make for really messy code, and in fact, Harold's next webrev is a
> really nice cleanup.
>
> Actually Klass is inherited from Metadata so is_Klass() is still useful
> for the cases where this is mixed. If we keep Klass::cast() we could use
> it for when the argument is (Metadata*), and continue to remove it from
> places where the type is already Klass*. I don't think this is worth
> doing because there are only a few of these cases in about 3 places.
> They already have casts and checks so it would be redundant there too.
>
> Coleen
>
> On 11/5/2012 1:37 AM, David Holmes wrote:
>> Hi Harold,
>>
>> On 3/11/2012 4:10 AM, harold seigel wrote:
>>> I don't see any value in the below assert. The compiler would have
>>> already ensured that any input argument is already of type Klass. If
>>> that isn't true then we would need to add asserts to every method
>>> verifying that its input arguments are really the type that they are
>>> declared to be.
>>
>> The assert does not check the type of the argument, it checks whether
>> the argument's is_Klass() method returns true. They are not the same
>> thing (you can pretty much cast anything to Klass* and pass it to
>> Klass::cast and the compiler will not complain).
>>
>> On the surface I agree with you that this seems redundant. But the
>> very fact this code is there tells me that there is more at play here
>> than meets the eye. See:
>>
>> https://jbs.oracle.com/bugs/browse/JDK-6992035
>> https://jbs.oracle.com/bugs/browse/JDK-4802822
>>
>> for cases where this assert failed. And there are others.
>>
>> As the saying goes "never take down a fence until you understand why
>> it was put up in the first place".
>>
>> Cheers,
>> David
>> ------
>>
>>>
>>> static Klass* cast(Klass* k) {
>>> * assert(k->is_klass(), "cast to Klass"); *
>>> return k;
>>> }
>>>
>>> For the asserts in InstanceKlass::cast(), note that the call to
>>> set_pool_holder() (from method.cpp) calls InstanceKlass::cast(). So the
>>> asserts will get exercised. The C++ compiler statically checks the rest
>>> of the cases.
>>>
>>> Thanks, Harold
>>>
>>> On 10/29/2012 10:55 PM, David Holmes wrote:
>>>> 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