RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

[David Holmes]

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


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

---

src/share/vm/jvmci/jvmciEnv.cpp

117     Klass*  kls;

This local is not needed as you can assign directly to found_klass now.

---

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?

---


> Also, fixing InstanceKlass to not pass this_k can be done in a separate
> cleanup.

Ok.

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
>