RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

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() == 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 
>