RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles (original) (raw)

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue Mar 14 00:01:49 UTC 2017

On 3/13/17 7:46 PM, David Holmes wrote:

On 13/03/2017 10:37 PM, 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 Oops I should have noticed I was re-checking the same .01 version :) Never mind. src/share/vm/jvmci/jvmciEnv.cpp 282 methodHandle JVMCIEnv::lookupmethod(InstanceKlass* haccessor, 283 InstanceKlass* hholder, the .hpp was fixed but not the .cpp.

got it!

Thanks!! Coleen

Otherwise I think they have all been fixed. Thanks, David

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 hik and klassh 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/c1Runtime1.cpp: InstanceKlass* h src/share/vm/ci/ciArrayKlass.cpp: Klass* hk src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* hk src/share/vm/ci/ciKlass.cpp/.hpp: Klass* hk src/share/vm/ci/ciMethod.cpp: Klass* hrecv, Klass* hresolved (though that code uses h when there doesn't appear to be a handle in use) src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* hk src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* hk src/share/vm/interpreter/interpreterRuntime.cpp: Klass* hklass, InstanceKlass* hcpentryf1 src/share/vm/prims/jni.cpp: Klass* hk src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ikh 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* instanceKh src/share/vm/prims/jvmtiEnvBase.cpp: Klass* obkh src/share/vm/prims/jvmtiExport.cpp: Klass* hclassbeingredefined src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* obkh src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh src/share/vm/prims/jvmtiThreadState.hpp: Klass* hclass src/share/vm/prims/nativeLookup.cpp: Klass* kh src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh src/share/vm/runtime/sharedRuntime.cpp: Klass* hklass src/share/vm/services/gcNotifier.cpp: InstanceKlass* mukh 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/c1Runtime1.cpp { Klass* klass = resolvefieldreturnklass(callermethod, bci, CHECK); - initklass = KlassHandle(THREAD, klass); + initklass = klass; You don't need the klass local but can assign direct to initklass. // convert to handle - loadklass = KlassHandle(THREAD, k); + loadklass = k; Comment should be deleted, plus it seems you should be able to assign to loadklass directly earlier through the switch instead of introducing 'k'. This logic is not obvious and I'd rather not change it. initklass and loadklass 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 foundklass 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 thisk 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