RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Mar 13 16:09:24 UTC 2017 

Hi Mikael,

On 3/13/17 11:21 AM, Mikael Gerdin wrote:
> Hi Coleen,
>
> On 2017-03-13 13:37, coleen.phillimore at oracle.com wrote:
>>
>> David, I'm so sorry, I should have pointed to
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev
>
> There are still a bunch of *Klass methods which are static and have an 
> InstanceKlass* (which used to be a handle) as their first parameter.
> Do you want to leave that to a future cleanup or fold it into this 
> change?
>
> Example:
> 1309   // Static methods that are used to implement member methods 
> where an exposed this pointer
> 1310   // is needed due to possible GCs
> 1311   static bool link_class_impl (instanceKlassHandle this_k, bool 
> throw_verifyerror, TRAPS);
> 1312   static bool verify_code (instanceKlassHandle this_k, bool 
> throw_verifyerror, TRAPS);
> 1313   static void initialize_impl (instanceKlassHandle this_k, TRAPS);
> 1314   static void initialize_super_interfaces (instanceKlassHandle 
> this_k, TRAPS);
> 1315   static void eager_initialize_impl (instanceKlassHandle this_k);
> 1316   static void set_initialization_state_and_notify_impl 
> (instanceKlassHandle this_k, ClassState state, TRAPS);
> 1317   static void call_class_initializer_impl (instanceKlassHandle 
> this_k, TRAPS);
> 1318   static Klass* array_klass_impl (instanceKlassHandle this_k, 
> bool or_null, int n, TRAPS);
> 1319   static void do_local_static_fields_impl (instanceKlassHandle 
> this_k, void f(fieldDescriptor* fd, Handle, TRAPS), Handle, TRAPS);
> 1320   /* jni_id_for_impl for jfieldID only */
> 1321   static JNIid* jni_id_for_impl (instanceKlassHandle this_k, int 
> offset);
> 1322
>
> All the above should probably be nonstatic member functions on 
> InstanceKlass instead but had to be made static since a permgen gc 
> could have moved "this" out from under the execution.

I wanted to leave this as a future cleanup, because I think the changes 
here would less cosmetic and probably would lead to more opportunity to 
clean things up.

Thanks - can I make you reviewer?

Coleen

>
> /Mikael
>
>>
>> On 3/13/17 2:23 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> Wow that was a hard slog! :)
>>>
>>> On 13/03/2017 2:18 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> As requested by David on the closed part of this review, I fixed
>>>> variable names of the form h_ik and klass_h where the 'h' indicated 
>>>> that
>>>> the klass was in a handle.
>>>
>>> Thanks for that. But I'm afraid there are quite a number of other
>>> patterns including the "h" still present:
>>>
>>> src/share/vm/aot/aotCodeHeap.cpp/.hpp: InstanceKlass* kh
>>> src/share/vm/c1/c1_Runtime1.cpp: InstanceKlass* h
>>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
>>> that code uses h_ when there doesn't appear to be a handle in use)
>>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>>> InstanceKlass* h_cp_entry_f1
>>> src/share/vm/prims/jni.cpp:  Klass* h_k
>>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>>> src/share/vm/prims/jvmtiClassFileReconstituter.cpp: InstanceKlass* iikh
>>> src/share/vm/prims/jvmtiClassFileReconstituter.hpp: InstanceKlass*
>>> _ikh, InstanceKlass*  ikh()
>>> src/share/vm/prims/jvmtiEnv.cpp: InstanceKlass* ikh, InstanceKlass*
>>> instanceK_h
>>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>>> src/share/vm/services/heapDumper.cpp: InstanceKlass* ikh
>>>
>>
>> These were the ones I fixed and retested.
>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.01/webrev
>>>
>>> There are numerous code indentation issues with some of the changed
>>> code (mainly around parameter/argument lists) - but they all seem to
>>> be pre-existing so I decided not to try and call them out. You did fix
>>> quite a few, and I don't think you introduced any new ones. :)
>>>
>>> The main thing to look for with use of handles internal to methods is
>>> whether once changed to a direct Klass/InstanceKlass*, the variable
>>> becomes redundant. You picked up on a few of these but I spotted a
>>> couple of others (reading the patch file the context isn't always
>>> sufficient to spot these):
>>>
>>> share/vm/c1/c1_Runtime1.cpp
>>>
>>>          { Klass* klass = resolve_field_return_klass(caller_method,
>>> bci, CHECK);
>>> -          init_klass = KlassHandle(THREAD, klass);
>>> +          init_klass = klass;
>>>
>>> You don't need the klass local but can assign direct to init_klass.
>>>
>>>      // convert to handle
>>> -    load_klass = KlassHandle(THREAD, k);
>>> +    load_klass = k;
>>>
>>> Comment should be deleted, plus it seems you should be able to assign
>>> to load_klass directly earlier through the switch instead of
>>> introducing 'k'.
>>
>> This logic is not obvious and I'd rather not change it. init_klass and
>> load_klass are used further down for different purposes.  I removed the
>> comment though.
>>>
>>> ---
>>>
>>> src/share/vm/jvmci/jvmciEnv.cpp
>>>
>>> 117     Klass*  kls;
>>>
>>> This local is not needed as you can assign directly to found_klass now.
>>
>> Yes, that's better.
>>>
>>> ---
>>>
>>> Finally a meta-question re the changes in:
>>>
>>> src/share/vm/classfile/dictionary.hpp
>>>
>>> and the flow out from that. I'm unclear about all the changes from
>>> Klass to InstanceKlass. Are all dictionary/hashtable entries
>>> guaranteed to be instance classes?
>>
>> Yes, they are.   This avoided multiple InstanceKlass::cast() calls.
>>>
>>> ---
>>>
>>>
>>>> Also, fixing InstanceKlass to not pass this_k can be done in a 
>>>> separate
>>>> cleanup.
>>>
>>> Ok.
>>
>> Thank you for slogging through all of this.
>>
>> Coleen
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 3/12/17 11:32 AM, coleen.phillimore at oracle.com wrote:
>>>>> Summary: Use unhandled pointers for Klass and InstanceKlass, remove
>>>>> handles with no implementation.
>>>>>
>>>>> These are fairly extensive changes.  The KlassHandle types have been
>>>>> dummy types since permgen elimination and were thought to be useful
>>>>> for future features.  They aren't, so can be removed (see bug for 
>>>>> more
>>>>> details).   This allows stricter typing because you can use the more
>>>>> specific type, and using const more.  I didn't add const to these
>>>>> changes, because of the cascading effect.
>>>>>
>>>>> The change isn't hard to review, just tedious.  The main bug that I
>>>>> had was redeclaring a type inside a scope, and InstanceKlass::cast(k)
>>>>> can't take a NULL k, whereas instanceKlassHandle ik(THREAD, k) could.
>>>>>
>>>>> It's so nice being able to single step on gdb without going into
>>>>> KlassHandle constructors!
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.01/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8155672
>>>>>
>>>>> Tested with all hotspot jtreg tests, java/lang/invoke tests,
>>>>> java/lang/instrument tests, all closed tonga colocated tests, and 
>>>>> JPRT.
>>>>>
>>>>> I can continue to hold this change for a convenient time for merging
>>>>> purposes with other people's JDK10 changes that they've been holding,
>>>>> or if there are other jdk9 changes that are likely to cause a problem
>>>>> for merging.  I'll update the copyrights to 2017 on commit.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>
>>

More information about the hotspot-dev mailing list