Quantcast

RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore

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, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

David Holmes
Hi Coleen,

Wow that was a hard slog! :)

On 13/03/2017 2:18 PM, [hidden email] 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.

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/c1_Runtime1.cpp: InstanceKlass* h
src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
that code uses h_ when there doesn't appear to be a handle in use)
src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
InstanceKlass* h_cp_entry_f1
src/share/vm/prims/jni.cpp:  Klass* h_k
src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
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*
instanceK_h
src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
src/share/vm/prims/jvmtiExport.cpp: Klass*  _h_class_being_redefined
src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
src/share/vm/prims/nativeLookup.cpp: Klass* kh
src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
src/share/vm/services/heapDumper.cpp: InstanceKlass* ikh


> 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/c1_Runtime1.cpp

          { Klass* klass = resolve_field_return_klass(caller_method,
bci, CHECK);
-          init_klass = KlassHandle(THREAD, klass);
+          init_klass = klass;

You don't need the klass local but can assign direct to init_klass.

      // convert to handle
-    load_klass = KlassHandle(THREAD, k);
+    load_klass = k;

Comment should be deleted, plus it seems you should be able to assign to
load_klass directly earlier through the switch instead of introducing 'k'.

---

src/share/vm/jvmci/jvmciEnv.cpp

117     Klass*  kls;

This local is not needed as you can assign directly to found_klass now.

---

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?

---


> Also, fixing InstanceKlass to not pass this_k can be done in a separate
> cleanup.

Ok.

Thanks,
David

> Thanks,
> Coleen
>
> On 3/12/17 11:32 AM, [hidden email] 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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore

David, I'm so sorry, I should have pointed to

open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev

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, [hidden email] 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.
>
> 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/c1_Runtime1.cpp: InstanceKlass* h
> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
> that code uses h_ when there doesn't appear to be a handle in use)
> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
> InstanceKlass* h_cp_entry_f1
> src/share/vm/prims/jni.cpp:  Klass* h_k
> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
> 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*
> instanceK_h
> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
> src/share/vm/prims/nativeLookup.cpp: Klass* kh
> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
> 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/c1_Runtime1.cpp
>
>          { Klass* klass = resolve_field_return_klass(caller_method,
> bci, CHECK);
> -          init_klass = KlassHandle(THREAD, klass);
> +          init_klass = klass;
>
> You don't need the klass local but can assign direct to init_klass.
>
>      // convert to handle
> -    load_klass = KlassHandle(THREAD, k);
> +    load_klass = k;
>
> Comment should be deleted, plus it seems you should be able to assign
> to load_klass directly earlier through the switch instead of
> introducing 'k'.

This logic is not obvious and I'd rather not change it.   init_klass and
load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

Lois Foltan


On 3/13/2017 8:37 AM, [hidden email] wrote:
>
> David, I'm so sorry, I should have pointed to
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev

Coleen,
Looks good.  A couple of instances of 'h' that I spotted during the review:

src/share/vm/aot - a couple of files still have 'h' in use
src/share/vm/classfile/systemDictionary.hpp -
handle_resolution_exception still has a parameter name of "klass_h"
src/share/vm/jvmci/jvmciRuntime.cpp - "InstanceKlass* h ="
src/share/vm/services/classLoadingService.hpp - is that really a
_klass_handle_array or now a _klass_array?

Lois

>
> 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, [hidden email] 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.
>>
>> 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/c1_Runtime1.cpp: InstanceKlass* h
>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved
>> (though that code uses h_ when there doesn't appear to be a handle in
>> use)
>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>> InstanceKlass* h_cp_entry_f1
>> src/share/vm/prims/jni.cpp:  Klass* h_k
>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>> 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*
>> instanceK_h
>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>> 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/c1_Runtime1.cpp
>>
>>          { Klass* klass = resolve_field_return_klass(caller_method,
>> bci, CHECK);
>> -          init_klass = KlassHandle(THREAD, klass);
>> +          init_klass = klass;
>>
>> You don't need the klass local but can assign direct to init_klass.
>>
>>      // convert to handle
>> -    load_klass = KlassHandle(THREAD, k);
>> +    load_klass = k;
>>
>> Comment should be deleted, plus it seems you should be able to assign
>> to load_klass directly earlier through the switch instead of
>> introducing 'k'.
>
> This logic is not obvious and I'd rather not change it. init_klass and
> load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

Mikael Gerdin
In reply to this post by coleen.phillimore
Hi Coleen,

On 2017-03-13 13:37, [hidden email] 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 link_class_impl
(instanceKlassHandle this_k, bool throw_verifyerror, TRAPS);
1312   static bool verify_code
(instanceKlassHandle this_k, bool throw_verifyerror, TRAPS);
1313   static void initialize_impl
(instanceKlassHandle this_k, TRAPS);
1314   static void initialize_super_interfaces
(instanceKlassHandle this_k, TRAPS);
1315   static void eager_initialize_impl
(instanceKlassHandle this_k);
1316   static void set_initialization_state_and_notify_impl
(instanceKlassHandle this_k, ClassState state, TRAPS);
1317   static void call_class_initializer_impl
(instanceKlassHandle this_k, TRAPS);
1318   static Klass* array_klass_impl
(instanceKlassHandle this_k, bool or_null, int n, TRAPS);
1319   static void do_local_static_fields_impl
(instanceKlassHandle this_k, void f(fieldDescriptor* fd, Handle, TRAPS),
Handle, TRAPS);
1320   /* jni_id_for_impl for jfieldID only */
1321   static JNIid* jni_id_for_impl
(instanceKlassHandle this_k, 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.

/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, [hidden email] 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.
>>
>> 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/c1_Runtime1.cpp: InstanceKlass* h
>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
>> that code uses h_ when there doesn't appear to be a handle in use)
>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>> InstanceKlass* h_cp_entry_f1
>> src/share/vm/prims/jni.cpp:  Klass* h_k
>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>> 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*
>> instanceK_h
>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>> 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/c1_Runtime1.cpp
>>
>>          { Klass* klass = resolve_field_return_klass(caller_method,
>> bci, CHECK);
>> -          init_klass = KlassHandle(THREAD, klass);
>> +          init_klass = klass;
>>
>> You don't need the klass local but can assign direct to init_klass.
>>
>>      // convert to handle
>> -    load_klass = KlassHandle(THREAD, k);
>> +    load_klass = k;
>>
>> Comment should be deleted, plus it seems you should be able to assign
>> to load_klass directly earlier through the switch instead of
>> introducing 'k'.
>
> This logic is not obvious and I'd rather not change it.   init_klass and
> load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore
In reply to this post by Lois Foltan


On 3/13/17 10:38 AM, Lois Foltan wrote:

>
>
> On 3/13/2017 8:37 AM, [hidden email] wrote:
>>
>> David, I'm so sorry, I should have pointed to
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev
>
> Coleen,
> Looks good.  A couple of instances of 'h' that I spotted during the
> review:
>
> src/share/vm/aot - a couple of files still have 'h' in use
> src/share/vm/classfile/systemDictionary.hpp -
> handle_resolution_exception still has a parameter name of "klass_h"
> src/share/vm/jvmci/jvmciRuntime.cpp - "InstanceKlass* h ="
> src/share/vm/services/classLoadingService.hpp - is that really a
> _klass_handle_array or now a _klass_array?

I fixed all of these to be ik, and classLoadingService now has a
_klass_array.

Thanks for reviewing this!
Coleen

>
> Lois
>
>>
>> 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, [hidden email] 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.
>>>
>>> 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/c1_Runtime1.cpp: InstanceKlass* h
>>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved
>>> (though that code uses h_ when there doesn't appear to be a handle
>>> in use)
>>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>>> InstanceKlass* h_cp_entry_f1
>>> src/share/vm/prims/jni.cpp:  Klass* h_k
>>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>>> 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*
>>> instanceK_h
>>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>>> 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/c1_Runtime1.cpp
>>>
>>>          { Klass* klass = resolve_field_return_klass(caller_method,
>>> bci, CHECK);
>>> -          init_klass = KlassHandle(THREAD, klass);
>>> +          init_klass = klass;
>>>
>>> You don't need the klass local but can assign direct to init_klass.
>>>
>>>      // convert to handle
>>> -    load_klass = KlassHandle(THREAD, k);
>>> +    load_klass = k;
>>>
>>> Comment should be deleted, plus it seems you should be able to
>>> assign to load_klass directly earlier through the switch instead of
>>> introducing 'k'.
>>
>> This logic is not obvious and I'd rather not change it. init_klass
>> and load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore
In reply to this post by Mikael Gerdin
Hi Mikael,

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

> Hi Coleen,
>
> On 2017-03-13 13:37, [hidden email] 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 link_class_impl (instanceKlassHandle this_k, bool
> throw_verifyerror, TRAPS);
> 1312   static bool verify_code (instanceKlassHandle this_k, bool
> throw_verifyerror, TRAPS);
> 1313   static void initialize_impl (instanceKlassHandle this_k, TRAPS);
> 1314   static void initialize_super_interfaces (instanceKlassHandle
> this_k, TRAPS);
> 1315   static void eager_initialize_impl (instanceKlassHandle this_k);
> 1316   static void set_initialization_state_and_notify_impl
> (instanceKlassHandle this_k, ClassState state, TRAPS);
> 1317   static void call_class_initializer_impl (instanceKlassHandle
> this_k, TRAPS);
> 1318   static Klass* array_klass_impl (instanceKlassHandle this_k,
> bool or_null, int n, TRAPS);
> 1319   static void do_local_static_fields_impl (instanceKlassHandle
> this_k, void f(fieldDescriptor* fd, Handle, TRAPS), Handle, TRAPS);
> 1320   /* jni_id_for_impl for jfieldID only */
> 1321   static JNIid* jni_id_for_impl (instanceKlassHandle this_k, 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, [hidden email] 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.
>>>
>>> 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/c1_Runtime1.cpp: InstanceKlass* h
>>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
>>> that code uses h_ when there doesn't appear to be a handle in use)
>>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>>> InstanceKlass* h_cp_entry_f1
>>> src/share/vm/prims/jni.cpp:  Klass* h_k
>>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>>> 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*
>>> instanceK_h
>>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>>> 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/c1_Runtime1.cpp
>>>
>>>          { Klass* klass = resolve_field_return_klass(caller_method,
>>> bci, CHECK);
>>> -          init_klass = KlassHandle(THREAD, klass);
>>> +          init_klass = klass;
>>>
>>> You don't need the klass local but can assign direct to init_klass.
>>>
>>>      // convert to handle
>>> -    load_klass = KlassHandle(THREAD, k);
>>> +    load_klass = k;
>>>
>>> Comment should be deleted, plus it seems you should be able to assign
>>> to load_klass directly earlier through the switch instead of
>>> introducing 'k'.
>>
>> This logic is not obvious and I'd rather not change it. init_klass and
>> load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

Mikael Gerdin
Hi Coleen,

On 2017-03-13 17:09, [hidden email] wrote:

> Hi Mikael,
>
> On 3/13/17 11:21 AM, Mikael Gerdin wrote:
>> Hi Coleen,
>>
>> On 2017-03-13 13:37, [hidden email] 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 link_class_impl (instanceKlassHandle this_k, bool
>> throw_verifyerror, TRAPS);
>> 1312   static bool verify_code (instanceKlassHandle this_k, bool
>> throw_verifyerror, TRAPS);
>> 1313   static void initialize_impl (instanceKlassHandle this_k, TRAPS);
>> 1314   static void initialize_super_interfaces (instanceKlassHandle
>> this_k, TRAPS);
>> 1315   static void eager_initialize_impl (instanceKlassHandle this_k);
>> 1316   static void set_initialization_state_and_notify_impl
>> (instanceKlassHandle this_k, ClassState state, TRAPS);
>> 1317   static void call_class_initializer_impl (instanceKlassHandle
>> this_k, TRAPS);
>> 1318   static Klass* array_klass_impl (instanceKlassHandle this_k,
>> bool or_null, int n, TRAPS);
>> 1319   static void do_local_static_fields_impl (instanceKlassHandle
>> this_k, void f(fieldDescriptor* fd, Handle, TRAPS), Handle, TRAPS);
>> 1320   /* jni_id_for_impl for jfieldID only */
>> 1321   static JNIid* jni_id_for_impl (instanceKlassHandle this_k, 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.

Ok, fine by me.

>
> Thanks - can I make you reviewer?

I only looked at instanceKlass.hpp/cpp to see if you had done the change
I asked about so I think not :)

Thanks for doing this cleanup!

/Mikael

>
> 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, [hidden email] 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.
>>>>
>>>> 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/c1_Runtime1.cpp: InstanceKlass* h
>>>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>>>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
>>>> that code uses h_ when there doesn't appear to be a handle in use)
>>>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>>>> InstanceKlass* h_cp_entry_f1
>>>> src/share/vm/prims/jni.cpp:  Klass* h_k
>>>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>>>> 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*
>>>> instanceK_h
>>>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>>>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>>>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>>>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>>>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>>>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>>>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>>>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>>>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>>>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>>>> 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/c1_Runtime1.cpp
>>>>
>>>>          { Klass* klass = resolve_field_return_klass(caller_method,
>>>> bci, CHECK);
>>>> -          init_klass = KlassHandle(THREAD, klass);
>>>> +          init_klass = klass;
>>>>
>>>> You don't need the klass local but can assign direct to init_klass.
>>>>
>>>>      // convert to handle
>>>> -    load_klass = KlassHandle(THREAD, k);
>>>> +    load_klass = k;
>>>>
>>>> Comment should be deleted, plus it seems you should be able to assign
>>>> to load_klass directly earlier through the switch instead of
>>>> introducing 'k'.
>>>
>>> This logic is not obvious and I'd rather not change it. init_klass and
>>> load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

Vladimir Ivanov
In reply to this post by coleen.phillimore
Coleen,

Glad to see klass handles finally being removed! Thanks for taking care
of that.

>>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev

I don't think you need the changes in doCall.cpp:

src/share/vm/opto/doCall.cpp:

-    ciInstanceKlass*    h_klass  = h->is_catch_all() ?
env()->Throwable_klass() : h->catch_klass();
+    ciInstanceKlass*    klass  = h->is_catch_all() ?
env()->Throwable_klass() : h->catch_klass();


> I fixed all of these to be ik, and classLoadingService now has a
> _klass_array.

Have you updated the webrev? Still see the following:

src/share/vm/aot/aotCodeHeap.hpp

+  bool load_klass_data(InstanceKlass* kh, Thread* thread);

src/share/vm/aot/aotCodeHeap.cpp:
+bool AOTCodeHeap::load_klass_data(InstanceKlass* kh, Thread* thread) {

src/share/vm/aot/aotLoader.cpp:

+void AOTLoader::load_for_klass(InstanceKlass* kh, Thread* thread) {

src/share/vm/c1/c1_Runtime1.cpp:
+  InstanceKlass* h = InstanceKlass::cast(klass);

src/share/vm/ci/ciKlass.hpp:
+  ciKlass(Klass* k_h, ciSymbol* name);

+  ciKlass(Klass* k_h);

src/share/vm/interpreter/interpreterRuntime.cpp:

+  InstanceKlass* h_cp_entry_f1 =
InstanceKlass::cast(cp_entry->f1_as_klass());

src/share/vm/jvmci/jvmciCompilerToVM.cpp:

+  Klass* h_resolved   = method->method_holder();

src/share/vm/jvmci/jvmciRuntime.cpp:

+  InstanceKlass* h = InstanceKlass::cast(klass);

src/share/vm/oops/instanceKlass.cpp:

+    InstanceKlass* ih = InstanceKlass::cast(interfaces->at(index));

src/share/vm/oops/objArrayKlass.cpp:

+ObjArrayKlass* ObjArrayKlass::allocate(ClassLoaderData* loader_data,
int n, Klass* klass_handle, Symbol* name, TRAPS) {

src/share/vm/prims/jvm.cpp:

+      InstanceKlass* k_h = InstanceKlass::cast(k);

src/share/vm/services/classLoadingService.hpp:

+  GrowableArray<Klass*>* _klass_handle_array;

src/share/vm/services/management.cpp:

+    Klass* kh = lce.get_klass(i);

Best regards,
Vladimir Ivanov

>>> 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, [hidden email] 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.
>>>>
>>>> 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/c1_Runtime1.cpp: InstanceKlass* h
>>>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>>>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved
>>>> (though that code uses h_ when there doesn't appear to be a handle
>>>> in use)
>>>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>>>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>>>> InstanceKlass* h_cp_entry_f1
>>>> src/share/vm/prims/jni.cpp:  Klass* h_k
>>>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>>>> 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*
>>>> instanceK_h
>>>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>>>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>>>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>>>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>>>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>>>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>>>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>>>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>>>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>>>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>>>> 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/c1_Runtime1.cpp
>>>>
>>>>          { Klass* klass = resolve_field_return_klass(caller_method,
>>>> bci, CHECK);
>>>> -          init_klass = KlassHandle(THREAD, klass);
>>>> +          init_klass = klass;
>>>>
>>>> You don't need the klass local but can assign direct to init_klass.
>>>>
>>>>      // convert to handle
>>>> -    load_klass = KlassHandle(THREAD, k);
>>>> +    load_klass = k;
>>>>
>>>> Comment should be deleted, plus it seems you should be able to
>>>> assign to load_klass directly earlier through the switch instead of
>>>> introducing 'k'.
>>>
>>> This logic is not obvious and I'd rather not change it. init_klass
>>> and load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore

Vladimir, thank you for reviewing this.  I'm glad someone from the
compiler group has looked at these changes.

On 3/13/17 12:37 PM, Vladimir Ivanov wrote:
> Coleen,
>
> Glad to see klass handles finally being removed! Thanks for taking
> care of that.

:)

>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev
>
> I don't think you need the changes in doCall.cpp:
>
> src/share/vm/opto/doCall.cpp:
>
> -    ciInstanceKlass*    h_klass  = h->is_catch_all() ?
> env()->Throwable_klass() : h->catch_klass();
> +    ciInstanceKlass*    klass  = h->is_catch_all() ?
> env()->Throwable_klass() : h->catch_klass();
>

I found those instances with the pattern matching.  I reverted doCall.cpp.
>
>> I fixed all of these to be ik, and classLoadingService now has a
>> _klass_array.
>
> Have you updated the webrev? Still see the following:

I haven't updated the webrev (it takes a while!)   I'll update after
this round of changes to .03.  Phew, you guys have good eyes.

See open webrev at http://cr.openjdk.java.net/~coleenp/8155672.03/webrev

that includes the changes below (in about an hour).

>
> src/share/vm/aot/aotCodeHeap.hpp
>
> +  bool load_klass_data(InstanceKlass* kh, Thread* thread);
>
> src/share/vm/aot/aotCodeHeap.cpp:
> +bool AOTCodeHeap::load_klass_data(InstanceKlass* kh, Thread* thread) {
>
> src/share/vm/aot/aotLoader.cpp:
>
> +void AOTLoader::load_for_klass(InstanceKlass* kh, Thread* thread) {
>
> src/share/vm/c1/c1_Runtime1.cpp:
> +  InstanceKlass* h = InstanceKlass::cast(klass);
>
> src/share/vm/ci/ciKlass.hpp:
> +  ciKlass(Klass* k_h, ciSymbol* name);
>
> +  ciKlass(Klass* k_h);

I missed this one.

>
> src/share/vm/interpreter/interpreterRuntime.cpp:
>
> +  InstanceKlass* h_cp_entry_f1 =
> InstanceKlass::cast(cp_entry->f1_as_klass());

Missed this one in my pattern matching.  Wasn't looking for h_ patterns,
only h_k, k_h, kh and h_klass.  fixed.

>
> src/share/vm/jvmci/jvmciCompilerToVM.cpp:
>
> +  Klass* h_resolved   = method->method_holder();

fixed.
>
> src/share/vm/jvmci/jvmciRuntime.cpp:
>
> +  InstanceKlass* h = InstanceKlass::cast(klass);

got this.
>
> src/share/vm/oops/instanceKlass.cpp:
>
> +    InstanceKlass* ih = InstanceKlass::cast(interfaces->at(index));

ih is not an easy thing to search for.  Changed to interk.

>
> src/share/vm/oops/objArrayKlass.cpp:
>
> +ObjArrayKlass* ObjArrayKlass::allocate(ClassLoaderData* loader_data,
> int n, Klass* klass_handle, Symbol* name, TRAPS) {

fixed.

>
> src/share/vm/prims/jvm.cpp:
>
> +      InstanceKlass* k_h = InstanceKlass::cast(k);
>
> src/share/vm/services/classLoadingService.hpp:
>
> +  GrowableArray<Klass*>* _klass_handle_array;
>
> src/share/vm/services/management.cpp:
>
> +    Klass* kh = lce.get_klass(i);

fixed.

Thanks!
Coleen

>
> Best regards,
> Vladimir Ivanov
>
>>>> 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, [hidden email] 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.
>>>>>
>>>>> 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/c1_Runtime1.cpp: InstanceKlass* h
>>>>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>>>>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>>>>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>>>>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved
>>>>> (though that code uses h_ when there doesn't appear to be a handle
>>>>> in use)
>>>>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>>>>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>>>>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>>>>> InstanceKlass* h_cp_entry_f1
>>>>> src/share/vm/prims/jni.cpp:  Klass* h_k
>>>>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>>>>> 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*
>>>>> instanceK_h
>>>>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>>>>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>>>>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>>>>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>>>>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>>>>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>>>>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>>>>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>>>>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>>>>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>>>>> 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/c1_Runtime1.cpp
>>>>>
>>>>>          { Klass* klass = resolve_field_return_klass(caller_method,
>>>>> bci, CHECK);
>>>>> -          init_klass = KlassHandle(THREAD, klass);
>>>>> +          init_klass = klass;
>>>>>
>>>>> You don't need the klass local but can assign direct to init_klass.
>>>>>
>>>>>      // convert to handle
>>>>> -    load_klass = KlassHandle(THREAD, k);
>>>>> +    load_klass = k;
>>>>>
>>>>> Comment should be deleted, plus it seems you should be able to
>>>>> assign to load_klass directly earlier through the switch instead of
>>>>> introducing 'k'.
>>>>
>>>> This logic is not obvious and I'd rather not change it. init_klass
>>>> and load_klass 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 found_klass
>>>>> 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 this_k 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, [hidden email] 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
>>>>>>
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

serguei.spitsyn@oracle.com
In reply to this post by coleen.phillimore
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.

Thanks,
Serguei


On 3/12/17 21:18, [hidden email] 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, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore


On 3/13/17 2:31 PM, [hidden email] 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?

thank you!
Coleen

>
> Thanks,
> Serguei
>
>
> On 3/12/17 21:18, [hidden email] 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, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

serguei.spitsyn@oracle.com
On 3/13/17 12:34, [hidden email] wrote:

>
>
> On 3/13/17 2:31 PM, [hidden email] 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

Thanks,
Serguei

>
> thank you!
> Coleen
>>
>> Thanks,
>> Serguei
>>
>>
>> On 3/12/17 21:18, [hidden email] 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, [hidden email] 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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore


On 3/13/17 3:42 PM, [hidden email] wrote:

> On 3/13/17 12:34, [hidden email] wrote:
>>
>>
>> On 3/13/17 2:31 PM, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

serguei.spitsyn@oracle.com
On 3/13/17 13:04, [hidden email] wrote:

>
>
> On 3/13/17 3:42 PM, [hidden email] wrote:
>> On 3/13/17 12:34, [hidden email] wrote:
>>>
>>>
>>> On 3/13/17 2:31 PM, [hidden email] 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.

Thanks!
Serguei


>
> Coleen
>
>>
>> Thanks,
>> Serguei
>>
>>>
>>> thank you!
>>> Coleen
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 3/12/17 21:18, [hidden email] 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, [hidden email] 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
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

David Holmes
In reply to this post by coleen.phillimore
On 13/03/2017 10:37 PM, [hidden email] wrote:
>
> David, I'm so sorry, I should have pointed to
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev

Oops I should have noticed I was re-checking the same .01 version :)

Never mind.

src/share/vm/jvmci/jvmciEnv.cpp

282 methodHandle JVMCIEnv::lookup_method(InstanceKlass* h_accessor,
283                                InstanceKlass* h_holder,

the .hpp was fixed but not the .cpp.

Otherwise I think they have all been fixed.

Thanks,
David

> 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, [hidden email] 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.
>>
>> 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/c1_Runtime1.cpp: InstanceKlass* h
>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
>> that code uses h_ when there doesn't appear to be a handle in use)
>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>> InstanceKlass* h_cp_entry_f1
>> src/share/vm/prims/jni.cpp:  Klass* h_k
>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>> 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*
>> instanceK_h
>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>> src/share/vm/prims/jvmtiExport.cpp: Klass*  _h_class_being_redefined
>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>> 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/c1_Runtime1.cpp
>>
>>          { Klass* klass = resolve_field_return_klass(caller_method,
>> bci, CHECK);
>> -          init_klass = KlassHandle(THREAD, klass);
>> +          init_klass = klass;
>>
>> You don't need the klass local but can assign direct to init_klass.
>>
>>      // convert to handle
>> -    load_klass = KlassHandle(THREAD, k);
>> +    load_klass = k;
>>
>> Comment should be deleted, plus it seems you should be able to assign
>> to load_klass directly earlier through the switch instead of
>> introducing 'k'.
>
> This logic is not obvious and I'd rather not change it.   init_klass and
> load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore


On 3/13/17 7:46 PM, David Holmes wrote:

> On 13/03/2017 10:37 PM, [hidden email] wrote:
>>
>> David, I'm so sorry, I should have pointed to
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.02/webrev
>
> Oops I should have noticed I was re-checking the same .01 version :)
>
> Never mind.
>
> src/share/vm/jvmci/jvmciEnv.cpp
>
> 282 methodHandle JVMCIEnv::lookup_method(InstanceKlass* h_accessor,
> 283                                InstanceKlass* h_holder,
>
> the .hpp was fixed but not the .cpp.

got it!

Thanks!!
Coleen

>
> Otherwise I think they have all been fixed.
>
> Thanks,
> David
>
>> 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, [hidden email] 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.
>>>
>>> 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/c1_Runtime1.cpp: InstanceKlass* h
>>> src/share/vm/ci/ciArrayKlass.cpp: Klass* h_k
>>> src/share/vm/ci/ciInstanceKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciMethod.cpp: Klass* h_recv, Klass* h_resolved (though
>>> that code uses h_ when there doesn't appear to be a handle in use)
>>> src/share/vm/ci/ciObjArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/ci/ciTypeArrayKlass.cpp/.hpp: Klass* h_k
>>> src/share/vm/interpreter/interpreterRuntime.cpp: Klass* h_klass,
>>> InstanceKlass* h_cp_entry_f1
>>> src/share/vm/prims/jni.cpp:  Klass* h_k
>>> src/share/vm/prims/jvm.cpp: InstanceKlass* kh, InstanceKlass* ik_h
>>> 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*
>>> instanceK_h
>>> src/share/vm/prims/jvmtiEnvBase.cpp: Klass* ob_kh
>>> src/share/vm/prims/jvmtiExport.cpp: Klass* _h_class_being_redefined
>>> src/share/vm/prims/jvmtiImpl.cpp: InstanceKlass* ikh, Klass* ob_kh
>>> src/share/vm/prims/jvmtiRedefineClasses.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiTagMap.cpp: InstanceKlass* ikh
>>> src/share/vm/prims/jvmtiThreadState.hpp: Klass* h_class
>>> src/share/vm/prims/nativeLookup.cpp: Klass* kh
>>> src/share/vm/prims/whitebox.cpp: InstanceKlass* ikh
>>> src/share/vm/runtime/sharedRuntime.cpp: Klass* h_klass
>>> src/share/vm/services/gcNotifier.cpp: InstanceKlass* mu_kh
>>> 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/c1_Runtime1.cpp
>>>
>>>          { Klass* klass = resolve_field_return_klass(caller_method,
>>> bci, CHECK);
>>> -          init_klass = KlassHandle(THREAD, klass);
>>> +          init_klass = klass;
>>>
>>> You don't need the klass local but can assign direct to init_klass.
>>>
>>>      // convert to handle
>>> -    load_klass = KlassHandle(THREAD, k);
>>> +    load_klass = k;
>>>
>>> Comment should be deleted, plus it seems you should be able to assign
>>> to load_klass directly earlier through the switch instead of
>>> introducing 'k'.
>>
>> This logic is not obvious and I'd rather not change it. init_klass and
>> load_klass 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 found_klass 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 this_k 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, [hidden email] 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
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

serguei.spitsyn@oracle.com
In reply to this post by coleen.phillimore
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, [hidden email] wrote:

> On 3/13/17 3:42 PM, [hidden email] wrote:
>> On 3/13/17 12:34, [hidden email] wrote:
>>> On 3/13/17 2:31 PM, [hidden email] 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, [hidden email] 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,
>>>>> [hidden email] 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles

coleen.phillimore

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

On 3/13/17 8:10 PM, [hidden email] 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, [hidden email] wrote:
>> On 3/13/17 3:42 PM, [hidden email] wrote:
>>> On 3/13/17 12:34, [hidden email] wrote:
>>>> On 3/13/17 2:31 PM, [hidden email] 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, [hidden email] 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,
>>>>>> [hidden email] 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

12
Loading...