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

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* hklass = h->iscatchall() ? env()->Throwableklass() : h->catchklass(); + ciInstanceKlass* klass = h->iscatchall() ? env()->Throwableklass() : h->catchklass();

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 klassarray. 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 loadklassdata(InstanceKlass* kh, Thread* thread); src/share/vm/aot/aotCodeHeap.cpp: +bool AOTCodeHeap::loadklassdata(InstanceKlass* kh, Thread* thread) { src/share/vm/aot/aotLoader.cpp: +void AOTLoader::loadforklass(InstanceKlass* kh, Thread* thread) { src/share/vm/c1/c1Runtime1.cpp: + InstanceKlass* h = InstanceKlass::cast(klass); src/share/vm/ci/ciKlass.hpp: + ciKlass(Klass* kh, ciSymbol* name); + ciKlass(Klass* kh);

I missed this one.

src/share/vm/interpreter/interpreterRuntime.cpp: + InstanceKlass* hcpentryf1 = InstanceKlass::cast(cpentry->f1asklass());

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* hresolved = method->methodholder();

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* loaderdata, int n, Klass* klasshandle, Symbol* name, TRAPS) {

fixed.

src/share/vm/prims/jvm.cpp: + InstanceKlass* kh = InstanceKlass::cast(k); src/share/vm/services/classLoadingService.hpp: + GrowableArray<Klass*>* klasshandlearray; src/share/vm/services/management.cpp: + Klass* kh = lce.getklass(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 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