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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Tue Mar 14 00:28:05 UTC 2017

Hi Coleen,

On 3/13/17 17:23, coleen.phillimore at oracle.com wrote:

Hi Serguei, Thank you for getting through this big change.

Amazing work! Thank you for making an update.

Thanks, Serguei

On 3/13/17 8:10 PM, serguei.spitsyn at oracle.com wrote: Coleen,

It looks good to me. Below are some minor comments. I'm Ok to separate the HandleMark cleanup if you prefer it. Yes, I would like to clean up the HandleMarks with a different change. They're somewhat complicated because they used to clean up Handles for oops and Handles for metadata. Now they only apply to oops. But some of the places below do accumulate oops so they might be needed there. I think Handle should be a scoped object with a destructor, etc so that we don't need HandleMarks, but I haven't filed an RFE for that yet. http://cr.openjdk.java.net/~coleenp/8155672.03/webrev/src/share/vm/oops/instanceKlass.cpp.frames.html

2581 if ((name() == innername)) { Unneeded extra brackets (existed before the fix). fixed. http://cr.openjdk.java.net/~coleenp/8155672.03/webrev/src/share/vm/oops/klassVtable.cpp.frames.html 1080 HandleMark hm(THREAD); Possibly a redundant HandleMark. http://cr.openjdk.java.net/~coleenp/8155672.03/webrev/src/share/vm/prims/jvmtiImpl.cpp.frames.html 695 HandleMark hm(curthread); Possibly a redundant HandleMark.

http://cr.openjdk.java.net/~coleenp/8155672.03/webrev/src/share/vm/prims/jvmtiRedefineClasses.cpp.frames.html 3853 InstanceKlass *ik = (InstanceKlass *)theclass; 4049 incrementclasscounter((InstanceKlass *)theclass, THREAD); No need to cast to (InstanceKlass ) as theclass isInstanceKlass . This was in commented out code but I fixed it. http://cr.openjdk.java.net/~coleenp/8155672.03/webrev/src/share/vm/runtime/vframe.cpp.frames.html 519 HandleMark hm; It seems to be redundant now. http://cr.openjdk.java.net/~coleenp/8155672.03/webrev/src/share/vm/services/classLoadingService.hpp.frames.html 124 // currentthread is for creating a Klass* with a faster version constructor 125 static Thread* currentthread; Does the currentthread become redundant now? Yes, it does! Good find. I removed this and am rerunning monitoring tests. http://cr.openjdk.java.net/~coleenp/8155672.03/webrev/src/share/vm/services/heapDumper.cpp.frames.html It seems, the HandleMark's at 816, 849, 879, 894 are redundant. These might have Handles for oops in them. TBD cleanup. Thank you for the code review, Coleen Thanks, Serguei On 3/13/17 13:04, coleen.phillimore at oracle.com wrote: On 3/13/17 3:42 PM, serguei.spitsyn at oracle.com wrote: On 3/13/17 12:34, coleen.phillimore at oracle.com wrote: On 3/13/17 2:31 PM, serguei.spitsyn at oracle.com wrote: Hi Coleen, It looks good. Nice cleanup! http://oklahoma.us.oracle.com/~cphillim/webrev/8155627.src.closed.02/webrev/share/vm/jfr/jfrClassAdapter.cpp.frames.html The HandleMark's at 1743 & 1784 can be not needed anymore. Thank you for reviewing! I suspect many HandleMarks lying around the code aren't needed anymore, because KlassHandles (and even methodHandles) aren't cleaned up using HandleMark. I've removed those two (and ran some retests because this is getting dangerous). Are you going to review the open too? Yes, but now I'm waiting for webrev version 03. This link is not resolved for me: http://cr.openjdk.java.net/~coleenp/8155672.03/webrev It's there now. I transposed the digits in the last webrev. Thanks for letting me know. Coleen Thanks, Serguei thank you! Coleen Thanks, Serguei On 3/12/17 21:18, 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. open webrev at http://cr.openjdk.java.net/~coleenp/8155672.01/webrev Also, fixing InstanceKlass to not pass thisk can be done in a separate cleanup. 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