RFR 8176705: Remove static functions in InstanceKlass

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

RFR 8176705: Remove static functions in InstanceKlass

harold seigel
Hi,

Please review this JDK-10 cleanup to convert certain static methods to
instance methods in class InstanceKlass.  These methods can now be
instance methods as a result of the fix for JDK-8155672
<https://bugs.openjdk.java.net/browse/JDK-8155672>.

Open Webrev:
http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html

JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705

The change was tested with the JCK lang and vm tests, the JTreg hotspot,
java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
tests, the colocated and non-colocated NSK tests, and with JPRT.

Thanks, Harold

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

Re: RFR 8176705: Remove static functions in InstanceKlass

Ioi Lam
Hi Harold,

Looks good. Small nits:

(1) Maybe do_local_static_fields_impl should be folded into
do_local_static_fields(void f(fieldDescriptor*, Handle, TRAPS), Handle
mirror, TRAPS)?

(2) Same for set_initialization_state_and_notify vs
set_initialization_state_and_notify_impl; and call_class_initializer vs
call_class_initializer_impl.

Thanks
- Ioi

On 3/19/17 10:41 PM, harold seigel wrote:

> Hi,
>
> Please review this JDK-10 cleanup to convert certain static methods to
> instance methods in class InstanceKlass.  These methods can now be
> instance methods as a result of the fix for JDK-8155672
> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>
> Open Webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>
> The change was tested with the JCK lang and vm tests, the JTreg
> hotspot, java/io, java/lang, java/util and other tests, the RBT tier2
> -tier5 tests, the colocated and non-colocated NSK tests, and with JPRT.
>
> Thanks, Harold
>

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

Re: RFR 8176705: Remove static functions in InstanceKlass

David Holmes
In reply to this post by harold seigel
Looks fine Harold!

Thanks,
David

On 20/03/2017 12:41 AM, harold seigel wrote:

> Hi,
>
> Please review this JDK-10 cleanup to convert certain static methods to
> instance methods in class InstanceKlass.  These methods can now be
> instance methods as a result of the fix for JDK-8155672
> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>
> Open Webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>
> The change was tested with the JCK lang and vm tests, the JTreg hotspot,
> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
> tests, the colocated and non-colocated NSK tests, and with JPRT.
>
> Thanks, Harold
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176705: Remove static functions in InstanceKlass

Mikael Gerdin
In reply to this post by harold seigel
Hi Harold,

On 2017-03-19 15:41, harold seigel wrote:
> Hi,
>
> Please review this JDK-10 cleanup to convert certain static methods to
> instance methods in class InstanceKlass.  These methods can now be
> instance methods as a result of the fix for JDK-8155672
> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>
> Open Webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html

Thanks for picking up this issue, I had a quick look and found some more
of these statics:

compute_enclosing_class/compute_enclosing_class_impl
find_inner_classes_attr
is_same_package_member
get_jmethod_id/get_jmethod_id_fetch_or_update

I also found this little jewel in instanceKlass.cpp:
2431 /* defined for now in jvm.cpp, for historical reasons *--
2432 Klass* InstanceKlass::compute_enclosing_class_impl(InstanceKlass* self,
2433                                                      Symbol*&
simple_name_result, TRAPS) {
2434   ...
2435 }
2436 */

Except the definition of compute_enclosing_class_impl is just a few
lines below the comment...

/Mikael

>
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>
> The change was tested with the JCK lang and vm tests, the JTreg hotspot,
> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
> tests, the colocated and non-colocated NSK tests, and with JPRT.
>
> Thanks, Harold
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176705: Remove static functions in InstanceKlass

harold seigel
In reply to this post by David Holmes
Hi David,

Thanks for the review!

Harold


On 3/19/2017 8:37 PM, David Holmes wrote:

> Looks fine Harold!
>
> Thanks,
> David
>
> On 20/03/2017 12:41 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-10 cleanup to convert certain static methods to
>> instance methods in class InstanceKlass.  These methods can now be
>> instance methods as a result of the fix for JDK-8155672
>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>
>> Open Webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>
>> The change was tested with the JCK lang and vm tests, the JTreg hotspot,
>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>
>> Thanks, Harold
>>

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

Re: RFR 8176705: Remove static functions in InstanceKlass

harold seigel
In reply to this post by Ioi Lam
Hi Ioi,

Thanks for your review!  I'll make those changes in the next webrev.

Harold


On 3/19/2017 8:36 PM, Ioi Lam wrote:

> Hi Harold,
>
> Looks good. Small nits:
>
> (1) Maybe do_local_static_fields_impl should be folded into
> do_local_static_fields(void f(fieldDescriptor*, Handle, TRAPS), Handle
> mirror, TRAPS)?
>
> (2) Same for set_initialization_state_and_notify vs
> set_initialization_state_and_notify_impl; and call_class_initializer
> vs call_class_initializer_impl.
>
> Thanks
> - Ioi
>
> On 3/19/17 10:41 PM, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-10 cleanup to convert certain static methods
>> to instance methods in class InstanceKlass.  These methods can now be
>> instance methods as a result of the fix for JDK-8155672
>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>
>> Open Webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>
>> The change was tested with the JCK lang and vm tests, the JTreg
>> hotspot, java/io, java/lang, java/util and other tests, the RBT tier2
>> -tier5 tests, the colocated and non-colocated NSK tests, and with JPRT.
>>
>> Thanks, Harold
>>
>

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

Re: RFR 8176705: Remove static functions in InstanceKlass

harold seigel
In reply to this post by Mikael Gerdin
Hi Mikael,

Thanks for the review!

Please review this updated webrev:
http://cr.openjdk.java.net/~hseigel/bug_8176705.2/webrev/index.html

It contains the changes suggested by you and Ioi.

Thanks, Harold


On 3/20/2017 5:34 AM, Mikael Gerdin wrote:

> Hi Harold,
>
> On 2017-03-19 15:41, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-10 cleanup to convert certain static methods to
>> instance methods in class InstanceKlass.  These methods can now be
>> instance methods as a result of the fix for JDK-8155672
>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>
>> Open Webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>
> Thanks for picking up this issue, I had a quick look and found some
> more of these statics:
>
> compute_enclosing_class/compute_enclosing_class_impl
> find_inner_classes_attr
> is_same_package_member
> get_jmethod_id/get_jmethod_id_fetch_or_update
>
> I also found this little jewel in instanceKlass.cpp:
> 2431 /* defined for now in jvm.cpp, for historical reasons *--
> 2432 Klass* InstanceKlass::compute_enclosing_class_impl(InstanceKlass*
> self,
> 2433 Symbol*& simple_name_result, TRAPS) {
> 2434   ...
> 2435 }
> 2436 */
>
> Except the definition of compute_enclosing_class_impl is just a few
> lines below the comment...
>
> /Mikael
>
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>
>> The change was tested with the JCK lang and vm tests, the JTreg hotspot,
>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>
>> Thanks, Harold
>>

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

Re: RFR 8176705: Remove static functions in InstanceKlass

Mikael Gerdin
Hi Harold,

On 2017-03-20 21:45, harold seigel wrote:
> Hi Mikael,
>
> Thanks for the review!
>
> Please review this updated webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8176705.2/webrev/index.html
>
> It contains the changes suggested by you and Ioi.

This looks great, thanks for cleaning this up!
Reviewed.

/Mikael

>
> Thanks, Harold
>
>
> On 3/20/2017 5:34 AM, Mikael Gerdin wrote:
>> Hi Harold,
>>
>> On 2017-03-19 15:41, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this JDK-10 cleanup to convert certain static methods to
>>> instance methods in class InstanceKlass.  These methods can now be
>>> instance methods as a result of the fix for JDK-8155672
>>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>>
>>> Open Webrev:
>>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>
>> Thanks for picking up this issue, I had a quick look and found some
>> more of these statics:
>>
>> compute_enclosing_class/compute_enclosing_class_impl
>> find_inner_classes_attr
>> is_same_package_member
>> get_jmethod_id/get_jmethod_id_fetch_or_update
>>
>> I also found this little jewel in instanceKlass.cpp:
>> 2431 /* defined for now in jvm.cpp, for historical reasons *--
>> 2432 Klass* InstanceKlass::compute_enclosing_class_impl(InstanceKlass*
>> self,
>> 2433 Symbol*& simple_name_result, TRAPS) {
>> 2434   ...
>> 2435 }
>> 2436 */
>>
>> Except the definition of compute_enclosing_class_impl is just a few
>> lines below the comment...
>>
>> /Mikael
>>
>>>
>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>>
>>> The change was tested with the JCK lang and vm tests, the JTreg hotspot,
>>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>>
>>> Thanks, Harold
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176705: Remove static functions in InstanceKlass

David Holmes
In reply to this post by harold seigel
Hi Harold,

On 21/03/2017 6:45 AM, harold seigel wrote:
> Hi Mikael,
>
> Thanks for the review!
>
> Please review this updated webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8176705.2/webrev/index.html
>
> It contains the changes suggested by you and Ioi.

src/share/vm/prims/jvm.cpp

Nit: no need to introduce the local variable.

Otherwise looks good!

Thanks,
David
-----


> Thanks, Harold
>
>
> On 3/20/2017 5:34 AM, Mikael Gerdin wrote:
>> Hi Harold,
>>
>> On 2017-03-19 15:41, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this JDK-10 cleanup to convert certain static methods to
>>> instance methods in class InstanceKlass.  These methods can now be
>>> instance methods as a result of the fix for JDK-8155672
>>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>>
>>> Open Webrev:
>>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>
>> Thanks for picking up this issue, I had a quick look and found some
>> more of these statics:
>>
>> compute_enclosing_class/compute_enclosing_class_impl
>> find_inner_classes_attr
>> is_same_package_member
>> get_jmethod_id/get_jmethod_id_fetch_or_update
>>
>> I also found this little jewel in instanceKlass.cpp:
>> 2431 /* defined for now in jvm.cpp, for historical reasons *--
>> 2432 Klass* InstanceKlass::compute_enclosing_class_impl(InstanceKlass*
>> self,
>> 2433 Symbol*& simple_name_result, TRAPS) {
>> 2434   ...
>> 2435 }
>> 2436 */
>>
>> Except the definition of compute_enclosing_class_impl is just a few
>> lines below the comment...
>>
>> /Mikael
>>
>>>
>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>>
>>> The change was tested with the JCK lang and vm tests, the JTreg hotspot,
>>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>>
>>> Thanks, Harold
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8176705: Remove static functions in InstanceKlass

harold seigel
Thanks David!

I'll remove the local before pushing the change.

Harold


On 3/21/2017 5:58 AM, David Holmes wrote:

> Hi Harold,
>
> On 21/03/2017 6:45 AM, harold seigel wrote:
>> Hi Mikael,
>>
>> Thanks for the review!
>>
>> Please review this updated webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8176705.2/webrev/index.html
>>
>> It contains the changes suggested by you and Ioi.
>
> src/share/vm/prims/jvm.cpp
>
> Nit: no need to introduce the local variable.
>
> Otherwise looks good!
>
> Thanks,
> David
> -----
>
>
>> Thanks, Harold
>>
>>
>> On 3/20/2017 5:34 AM, Mikael Gerdin wrote:
>>> Hi Harold,
>>>
>>> On 2017-03-19 15:41, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this JDK-10 cleanup to convert certain static methods to
>>>> instance methods in class InstanceKlass.  These methods can now be
>>>> instance methods as a result of the fix for JDK-8155672
>>>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>>>
>>>> Open Webrev:
>>>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>>
>>> Thanks for picking up this issue, I had a quick look and found some
>>> more of these statics:
>>>
>>> compute_enclosing_class/compute_enclosing_class_impl
>>> find_inner_classes_attr
>>> is_same_package_member
>>> get_jmethod_id/get_jmethod_id_fetch_or_update
>>>
>>> I also found this little jewel in instanceKlass.cpp:
>>> 2431 /* defined for now in jvm.cpp, for historical reasons *--
>>> 2432 Klass* InstanceKlass::compute_enclosing_class_impl(InstanceKlass*
>>> self,
>>> 2433 Symbol*& simple_name_result, TRAPS) {
>>> 2434   ...
>>> 2435 }
>>> 2436 */
>>>
>>> Except the definition of compute_enclosing_class_impl is just a few
>>> lines below the comment...
>>>
>>> /Mikael
>>>
>>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>>>
>>>> The change was tested with the JCK lang and vm tests, the JTreg
>>>> hotspot,
>>>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>>>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>>>
>>>> Thanks, Harold
>>>>
>>

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

Re: RFR 8176705: Remove static functions in InstanceKlass

harold seigel
In reply to this post by Mikael Gerdin
Thanks Mikael!

Harold


On 3/21/2017 5:26 AM, Mikael Gerdin wrote:

> Hi Harold,
>
> On 2017-03-20 21:45, harold seigel wrote:
>> Hi Mikael,
>>
>> Thanks for the review!
>>
>> Please review this updated webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8176705.2/webrev/index.html
>>
>> It contains the changes suggested by you and Ioi.
>
> This looks great, thanks for cleaning this up!
> Reviewed.
>
> /Mikael
>
>>
>> Thanks, Harold
>>
>>
>> On 3/20/2017 5:34 AM, Mikael Gerdin wrote:
>>> Hi Harold,
>>>
>>> On 2017-03-19 15:41, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this JDK-10 cleanup to convert certain static methods to
>>>> instance methods in class InstanceKlass.  These methods can now be
>>>> instance methods as a result of the fix for JDK-8155672
>>>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>>>
>>>> Open Webrev:
>>>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>>
>>> Thanks for picking up this issue, I had a quick look and found some
>>> more of these statics:
>>>
>>> compute_enclosing_class/compute_enclosing_class_impl
>>> find_inner_classes_attr
>>> is_same_package_member
>>> get_jmethod_id/get_jmethod_id_fetch_or_update
>>>
>>> I also found this little jewel in instanceKlass.cpp:
>>> 2431 /* defined for now in jvm.cpp, for historical reasons *--
>>> 2432 Klass* InstanceKlass::compute_enclosing_class_impl(InstanceKlass*
>>> self,
>>> 2433 Symbol*& simple_name_result, TRAPS) {
>>> 2434   ...
>>> 2435 }
>>> 2436 */
>>>
>>> Except the definition of compute_enclosing_class_impl is just a few
>>> lines below the comment...
>>>
>>> /Mikael
>>>
>>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>>>
>>>> The change was tested with the JCK lang and vm tests, the JTreg
>>>> hotspot,
>>>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>>>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>>>
>>>> Thanks, Harold
>>>>
>>

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

Re: RFR 8176705: Remove static functions in InstanceKlass

Ioi Lam
In reply to this post by harold seigel
Looks good. Thanks Harold.

- Ioi

On 3/21/17 4:45 AM, harold seigel wrote:

> Hi Mikael,
>
> Thanks for the review!
>
> Please review this updated webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8176705.2/webrev/index.html
>
> It contains the changes suggested by you and Ioi.
>
> Thanks, Harold
>
>
> On 3/20/2017 5:34 AM, Mikael Gerdin wrote:
>> Hi Harold,
>>
>> On 2017-03-19 15:41, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this JDK-10 cleanup to convert certain static methods to
>>> instance methods in class InstanceKlass.  These methods can now be
>>> instance methods as a result of the fix for JDK-8155672
>>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>>
>>> Open Webrev:
>>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>
>> Thanks for picking up this issue, I had a quick look and found some
>> more of these statics:
>>
>> compute_enclosing_class/compute_enclosing_class_impl
>> find_inner_classes_attr
>> is_same_package_member
>> get_jmethod_id/get_jmethod_id_fetch_or_update
>>
>> I also found this little jewel in instanceKlass.cpp:
>> 2431 /* defined for now in jvm.cpp, for historical reasons *--
>> 2432 Klass*
>> InstanceKlass::compute_enclosing_class_impl(InstanceKlass* self,
>> 2433 Symbol*& simple_name_result, TRAPS) {
>> 2434   ...
>> 2435 }
>> 2436 */
>>
>> Except the definition of compute_enclosing_class_impl is just a few
>> lines below the comment...
>>
>> /Mikael
>>
>>>
>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>>
>>> The change was tested with the JCK lang and vm tests, the JTreg
>>> hotspot,
>>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>>
>>> Thanks, Harold
>>>
>

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

Re: RFR 8176705: Remove static functions in InstanceKlass

harold seigel
Thanks Ioi!

Harold


On 3/21/2017 11:15 AM, Ioi Lam wrote:

> Looks good. Thanks Harold.
>
> - Ioi
>
> On 3/21/17 4:45 AM, harold seigel wrote:
>> Hi Mikael,
>>
>> Thanks for the review!
>>
>> Please review this updated webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8176705.2/webrev/index.html
>>
>> It contains the changes suggested by you and Ioi.
>>
>> Thanks, Harold
>>
>>
>> On 3/20/2017 5:34 AM, Mikael Gerdin wrote:
>>> Hi Harold,
>>>
>>> On 2017-03-19 15:41, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this JDK-10 cleanup to convert certain static methods to
>>>> instance methods in class InstanceKlass.  These methods can now be
>>>> instance methods as a result of the fix for JDK-8155672
>>>> <https://bugs.openjdk.java.net/browse/JDK-8155672>.
>>>>
>>>> Open Webrev:
>>>> http://cr.openjdk.java.net/~hseigel/bug_8176705/webrev/index.html
>>>
>>> Thanks for picking up this issue, I had a quick look and found some
>>> more of these statics:
>>>
>>> compute_enclosing_class/compute_enclosing_class_impl
>>> find_inner_classes_attr
>>> is_same_package_member
>>> get_jmethod_id/get_jmethod_id_fetch_or_update
>>>
>>> I also found this little jewel in instanceKlass.cpp:
>>> 2431 /* defined for now in jvm.cpp, for historical reasons *--
>>> 2432 Klass*
>>> InstanceKlass::compute_enclosing_class_impl(InstanceKlass* self,
>>> 2433 Symbol*& simple_name_result, TRAPS) {
>>> 2434   ...
>>> 2435 }
>>> 2436 */
>>>
>>> Except the definition of compute_enclosing_class_impl is just a few
>>> lines below the comment...
>>>
>>> /Mikael
>>>
>>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8176705
>>>>
>>>> The change was tested with the JCK lang and vm tests, the JTreg
>>>> hotspot,
>>>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>>>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>>>
>>>> Thanks, Harold
>>>>
>>
>

Loading...