RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

Tue Mar 14 00:23:06 UTC 2017

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

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() == inner_name)) {
>
>   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(cur_thread);
>
>  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 *)the_class; 4049 
> increment_class_counter((InstanceKlass *)the_class, THREAD);
>
>  No need to cast to (InstanceKlass *) as the_class 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 // _current_thread is for creating a Klass* with a faster version 
> constructor
>  125   static Thread*                     _current_thread;
>
> Does the _current_thread 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 h_ik and klass_h 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 this_k 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 