RFR: 8035746: Add missing Klass::oop_is_instanceClassLoader() function

[Stefan Karlsson]

On 2014-02-26 22:16, Coleen Phillimore wrote:
>
> Okay with below.  Thanks for pointing out the code that could be 
> changed - I agree, they are not really worth doing.

Thanks again, Coleen.

StefanK

> Coleen
>
>
> On 2/26/2014 3:32 PM, Stefan Karlsson wrote:
>> On 2014-02-26 20:38, Coleen Phillimore wrote:
>>> On 2/25/2014 12:50 PM, Stefan Karlsson wrote:
>>>> On 2014-02-25 15:15, Mikael Gerdin wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> On Tuesday 25 February 2014 14.43.49 Stefan Karlsson wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please, review this small patch to add 
>>>>>> Klass::oop_is_instanceClassLoader().
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~stefank/8035746/webrev.00
>>>>> Code change looks good.
>>>>
>>>> Thanks.
>>>>
>>>>>
>>>>> There are a couple of places where we check whether or not a Klass 
>>>>> is a
>>>>> ClassLoader, something like:
>>>>> if (klass->is_subclass_of(SystemDictionary::ClassLoader_klass()))
>>>>>
>>>>> These places could now be changed to
>>>>> klass->oop_is_ClassLoader(), saving a possibly expensive subtype 
>>>>> check.
>>>>>
>>>>> Should we file an RFE to change these or do you want to change 
>>>>> them straight
>>>>> away?
>>>>
>>>> I'd prefer a separate RFE.
>>>
>>> This change looks good.
>>
>> Thanks.
>>
>>>   I assume this is actually for G1 class unloading.
>>
>> Yes.
>>
>>> Too bad though I wish you'd change it all in one pass.
>>
>> I just wanted to bring over this small change from the G1 Class 
>> Unloading repository and not have to argue whether changing the 
>> usages is_subclass_of to oop_is_instanceClassLoader was worth it or not.
>>
>> I did a quick search for this and found these three usages:
>>
>> 1)
>> src//share/vm/classfile/javaClasses.hpp-  static bool 
>> is_subclass(Klass* klass) {
>> src//share/vm/classfile/javaClasses.hpp:    return 
>> klass->is_subclass_of(SystemDictionary::ClassLoader_klass());
>> src//share/vm/classfile/javaClasses.hpp-  }
>>
>> which is unused, AFAICT.
>>
>> 2)
>> src//share/vm/prims/jvm.cpp:      if 
>> (!vfst.method()->method_holder()->is_subclass_of(SystemDictionary::ClassLoader_klass())&&
>> src//share/vm/prims/jvm.cpp- 
>> !vfst.method()->method_holder()->is_subclass_of(access_controller_klass) 
>> &&
>> src//share/vm/prims/jvm.cpp- 
>> !vfst.method()->method_holder()->is_subclass_of(privileged_action_klass)) 
>> {
>>
>> I don't think this helps the readability of the code if we change it. 
>> I don't know if this is performance critical.
>>
>> thanks,
>> StefanK
>>
>>> Thanks,
>>> Coleen
>>>
>>>>
>>>> thanks,
>>>> StefanK
>>>>
>>>>>
>>>>> /Mikael
>>>>>
>>>>>> RFE:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8035746
>>>>>>
>>>>>> thanks,
>>>>>> StefanK
>>>>
>>>
>>
>