RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Mar 13 17:38:27 UTC 2017 

Vladimir, thank you for reviewing this.  I'm glad someone from the 
compiler group has looked at these changes.

On 3/13/17 12:37 PM, Vladimir Ivanov wrote:
> 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 found those instances with the pattern matching.  I reverted doCall.cpp.
>
>> I fixed all of these to be ik, and classLoadingService now has a
>> _klass_array.
>
> Have you updated the webrev? Still see the following:

I haven't updated the webrev (it takes a while!)   I'll update after 
this round of changes to .03.  Phew, you guys have good eyes.

See open webrev at http://cr.openjdk.java.net/~coleenp/8155672.03/webrev

that includes the changes below (in about an hour).
>
> 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);

I missed this one.

>
> src/share/vm/interpreter/interpreterRuntime.cpp:
>
> +  InstanceKlass* h_cp_entry_f1 = 
> InstanceKlass::cast(cp_entry->f1_as_klass());

Missed this one in my pattern matching.  Wasn't looking for h_ patterns, 
only h_k, k_h, kh and h_klass.  fixed.

>
> src/share/vm/jvmci/jvmciCompilerToVM.cpp:
>
> +  Klass* h_resolved   = method->method_holder();

fixed.
>
> src/share/vm/jvmci/jvmciRuntime.cpp:
>
> +  InstanceKlass* h = InstanceKlass::cast(klass);

got this.
>
> src/share/vm/oops/instanceKlass.cpp:
>
> +    InstanceKlass* ih = InstanceKlass::cast(interfaces->at(index));

ih is not an easy thing to search for.  Changed to interk.

>
> src/share/vm/oops/objArrayKlass.cpp:
>
> +ObjArrayKlass* ObjArrayKlass::allocate(ClassLoaderData* loader_data, 
> int n, Klass* klass_handle, Symbol* name, TRAPS) {

fixed.
>
> 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);

fixed.

Thanks!
Coleen
>
> 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