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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Mar 13 16:09:24 UTC 2017

Hi Mikael,

On 3/13/17 11:21 AM, Mikael Gerdin wrote:

Hi Coleen,

On 2017-03-13 13:37, 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 There are still a bunch of *Klass methods which are static and have an InstanceKlass* (which used to be a handle) as their first parameter. Do you want to leave that to a future cleanup or fold it into this change? Example: 1309 // Static methods that are used to implement member methods where an exposed this pointer 1310 // is needed due to possible GCs 1311 static bool linkclassimpl (instanceKlassHandle thisk, bool throwverifyerror, TRAPS); 1312 static bool verifycode (instanceKlassHandle thisk, bool throwverifyerror, TRAPS); 1313 static void initializeimpl (instanceKlassHandle thisk, TRAPS); 1314 static void initializesuperinterfaces (instanceKlassHandle thisk, TRAPS); 1315 static void eagerinitializeimpl (instanceKlassHandle thisk); 1316 static void setinitializationstateandnotifyimpl (instanceKlassHandle thisk, ClassState state, TRAPS); 1317 static void callclassinitializerimpl (instanceKlassHandle thisk, TRAPS); 1318 static Klass* arrayklassimpl (instanceKlassHandle thisk, bool ornull, int n, TRAPS); 1319 static void dolocalstaticfieldsimpl (instanceKlassHandle thisk, void f(fieldDescriptor* fd, Handle, TRAPS), Handle, TRAPS); 1320 /* jniidforimpl for jfieldID only */ 1321 static JNIid* jniidforimpl (instanceKlassHandle thisk, int offset); 1322 All the above should probably be nonstatic member functions on InstanceKlass instead but had to be made static since a permgen gc could have moved "this" out from under the execution.

I wanted to leave this as a future cleanup, because I think the changes here would less cosmetic and probably would lead to more opportunity to clean things up.

Thanks - can I make you reviewer?

Coleen

/Mikael

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