RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

Tue Mar 14 00:10:12 UTC 2017

Coleen,

It looks good to me.
Below are some minor comments.
I'm Ok to separate the HandleMark cleanup if you prefer it.

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).

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* . 
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?

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. 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 