RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

[Lois Foltan]

On 3/13/2017 8:37 AM, 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

Coleen,
Looks good.  A couple of instances of 'h' that I spotted during the review:

src/share/vm/aot - a couple of files still have 'h' in use
src/share/vm/classfile/systemDictionary.hpp - 
handle_resolution_exception still has a parameter name of "klass_h"
src/share/vm/jvmci/jvmciRuntime.cpp - "InstanceKlass* h ="
src/share/vm/services/classLoadingService.hpp - is that really a 
_klass_handle_array or now a _klass_array?

Lois

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