RFR (XL) 8155672: Remove instanceKlassHandles and KlassHandles
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Mar 13 20:05:44 UTC 2017
On 3/13/17 13:04, coleen.phillimore at oracle.com wrote:
>
>
> On 3/13/17 3:42 PM, serguei.spitsyn at oracle.com wrote:
>> On 3/13/17 12:34, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 3/13/17 2:31 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Coleen,
>>>>
>>>> It looks good.
>>>> Nice cleanup!
>>>>
>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8155627.src.closed.02/webrev/share/vm/jfr/jfrClassAdapter.cpp.frames.html
>>>>
>>>>
>>>> The HandleMark's at 1743 & 1784 can be not needed anymore.
>>>
>>> Thank you for reviewing! I suspect many HandleMarks lying around
>>> the code aren't needed anymore, because KlassHandles (and even
>>> methodHandles) aren't cleaned up using HandleMark. I've removed
>>> those two (and ran some retests because this is getting dangerous).
>>>
>>> Are you going to review the open too?
>>
>> Yes, but now I'm waiting for webrev version 03.
>> This link is not resolved for me:
>> http://cr.openjdk.java.net/~coleenp/8155672.03/webrev
>
> It's there now. I transposed the digits in the last webrev. Thanks
> for letting me know.
Thanks!
Serguei
>
> Coleen
>
>>
>> Thanks,
>> Serguei
>>
>>>
>>> thank you!
>>> Coleen
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 3/12/17 21:18, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> As requested by David on the closed part of this review, I fixed
>>>>> variable names of the form h_ik and klass_h where the 'h'
>>>>> indicated that the klass was in a handle.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.01/webrev
>>>>>
>>>>> Also, fixing InstanceKlass to not pass this_k can be done in a
>>>>> separate cleanup.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 3/12/17 11:32 AM, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: Use unhandled pointers for Klass and InstanceKlass,
>>>>>> remove handles with no implementation.
>>>>>>
>>>>>> These are fairly extensive changes. The KlassHandle types have
>>>>>> been dummy types since permgen elimination and were thought to be
>>>>>> useful for future features. They aren't, so can be removed (see
>>>>>> bug for more details). This allows stricter typing because you
>>>>>> can use the more specific type, and using const more. I didn't
>>>>>> add const to these changes, because of the cascading effect.
>>>>>>
>>>>>> The change isn't hard to review, just tedious. The main bug that
>>>>>> I had was redeclaring a type inside a scope, and
>>>>>> InstanceKlass::cast(k) can't take a NULL k, whereas
>>>>>> instanceKlassHandle ik(THREAD, k) could.
>>>>>>
>>>>>> It's so nice being able to single step on gdb without going into
>>>>>> KlassHandle constructors!
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8155672.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8155672
>>>>>>
>>>>>> Tested with all hotspot jtreg tests, java/lang/invoke tests,
>>>>>> java/lang/instrument tests, all closed tonga colocated tests, and
>>>>>> JPRT.
>>>>>>
>>>>>> I can continue to hold this change for a convenient time for
>>>>>> merging purposes with other people's JDK10 changes that they've
>>>>>> been holding, or if there are other jdk9 changes that are likely
>>>>>> to cause a problem for merging. I'll update the copyrights to
>>>>>> 2017 on commit.
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list