RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Mon Mar 13 16:37:23 UTC 2017 

Coleen,

Glad to see klass handles finally being removed! Thanks for taking care 
of that.

>>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev

I don't think you need the changes in doCall.cpp:

src/share/vm/opto/doCall.cpp:

-    ciInstanceKlass*    h_klass  = h->is_catch_all() ? 
env()->Throwable_klass() : h->catch_klass();
+    ciInstanceKlass*    klass  = h->is_catch_all() ? 
env()->Throwable_klass() : h->catch_klass();


> I fixed all of these to be ik, and classLoadingService now has a
> _klass_array.

Have you updated the webrev? Still see the following:

src/share/vm/aot/aotCodeHeap.hpp

+  bool load_klass_data(InstanceKlass* kh, Thread* thread);

src/share/vm/aot/aotCodeHeap.cpp:
+bool AOTCodeHeap::load_klass_data(InstanceKlass* kh, Thread* thread) {

src/share/vm/aot/aotLoader.cpp:

+void AOTLoader::load_for_klass(InstanceKlass* kh, Thread* thread) {

src/share/vm/c1/c1_Runtime1.cpp:
+  InstanceKlass* h = InstanceKlass::cast(klass);

src/share/vm/ci/ciKlass.hpp:
+  ciKlass(Klass* k_h, ciSymbol* name);

+  ciKlass(Klass* k_h);

src/share/vm/interpreter/interpreterRuntime.cpp:

+  InstanceKlass* h_cp_entry_f1 = 
InstanceKlass::cast(cp_entry->f1_as_klass());

src/share/vm/jvmci/jvmciCompilerToVM.cpp:

+  Klass* h_resolved   = method->method_holder();

src/share/vm/jvmci/jvmciRuntime.cpp:

+  InstanceKlass* h = InstanceKlass::cast(klass);

src/share/vm/oops/instanceKlass.cpp:

+    InstanceKlass* ih = InstanceKlass::cast(interfaces->at(index));

src/share/vm/oops/objArrayKlass.cpp:

+ObjArrayKlass* ObjArrayKlass::allocate(ClassLoaderData* loader_data, 
int n, Klass* klass_handle, Symbol* name, TRAPS) {

src/share/vm/prims/jvm.cpp:

+      InstanceKlass* k_h = InstanceKlass::cast(k);

src/share/vm/services/classLoadingService.hpp:

+  GrowableArray<Klass*>* _klass_handle_array;

src/share/vm/services/management.cpp:

+    Klass* kh = lce.get_klass(i);

Best regards,
Vladimir Ivanov

>>> 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