RFR(s): 8235912: JvmtiBreakpoint remove oops_do and metadata_do

[coleen.phillimore at oracle.com]

Hi Serguei,
The review thread should be "RFR 8235829: graal crashes with Zombie.java 
test".

On 12/17/19 8:17 PM, serguei.spitsyn at oracle.com wrote:
> Hi Coleen,
>
> Is this webrev v2 right to look at? :
> http://cr.openjdk.java.net/~coleenp/2019/8235829.02/webrev/
>
Actually, this one was something I tried that didn't work, so the review 
was 01.   I created 02 with the change to make 
run_nmethod_entry_barriers plural:

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

Can you review this one on the other thread?

Thanks,
Coleen

> It looks good to me.
> Just one nit (sorry, if it is a duplicated comment):
>
> http://cr.openjdk.java.net/~coleenp/2019/8235829.02/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.frames.html
> 1066 void JvmtiDeferredEventQueue::run_nmethod_entry_barrier() {
> 1067 for(QueueNode* node = _queue_head; node != NULL; node = 
> node->next()) {
> 1068 node->event().run_nmethod_entry_barrier();
> 1069 }
> 1070 }
> The function run_nmethod_entry_barrier() should have a plural form as 
> it iterates over the queue.
>
>
>
> Thanks
> Serguei
>
>
> On 12/17/19 5:59 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 12/17/19 4:21 AM, Robbin Ehn wrote:
>>> Hi Coleen,
>>>
>>> On 12/16/19 9:21 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> http://cr.openjdk.java.net/~rehn/8235912/v1/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.udiff.html 
>>>>
>>>>
>>>> + NativeAccess<>::oop_store(_class_holder, class_holder_oop);
>>>>
>>>>
>>>> This should probably be:
>>>>
>>>>    41 NativeAccess<IS_DEST_UNINITIALIZED>::oop_store(handle, obj);
>>>>
>>>
>>> I have not seen any stores to oopStorage that use that?
>>> oopStorage should be 'initialized'.
>>> So I prefer not adding another decorator if it's not needed.
>>> That would just be confusing.
>>
>> It's in the ClassLoaderData initialization.  I don't know what it 
>> means, you'd have to ask someone who knows.  I don't think it matters 
>> though.
>>
>> Coleen
>>>
>>>>
>>>> You can leave out using OopHandle.  I have a patch to add the 
>>>> missing functionality and add it to your code. Actually, I was 
>>>> looking to see how much OopHandle is used to see if it's helping 
>>>> anything and there is a lot of code using it.  Most of it is to 
>>>> hide oop* in ClassLoaderData.
>>>>
>>>> This change otherwise looks great.
>>>
>>> Thanks, Robbin
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>>> Thanks for having a look, Robbin
>>>>>
>>>>> On 12/16/19 1:32 PM, coleen.phillimore at oracle.com wrote:
>>>>>>
>>>>>> I have to think about this.   Could there be breakpoints in old 
>>>>>> emcp methods that we do not remove?   The metadata_do function is 
>>>>>> trying to keep old Methods from being deleted while there are 
>>>>>> still references to them.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~rehn/8235912/v1/webrev/src/hotspot/share/prims/jvmtiImpl.hpp.udiff.html 
>>>>>>
>>>>>>
>>>>>> + oop* _class_holder; // keeps _method memory from being deallocated
>>>>>>
>>>>>>
>>>>>> We created the class OopHandle to encapsulate strong oopStorage 
>>>>>> references, although it's missing oop_store. Can you use that?
>>>>>
>>>>>
>>>>>>
>>>>>> Coleen
>>>>>>
>>>>>> On 12/16/19 4:47 AM, Robbin Ehn wrote:
>>>>>>> Hi all, please review.
>>>>>>>
>>>>>>> From issue, https://bugs.openjdk.java.net/browse/JDK-8235912:
>>>>>>>
>>>>>>> JvmtiBreakpoints are walked via VMThread oops_do (the breakpoint 
>>>>>>> is in a vm operation) before they are installed in the safeopint 
>>>>>>> and after they have been installed, walked with 
>>>>>>> JvmtiCurrentBreakpoints::oops_do().
>>>>>>> By putting the class holder inside oopStorage there is no need 
>>>>>>> for this.
>>>>>>>
>>>>>>> JvmtiCurrentBreakpoints::metadata_do is not needed because 
>>>>>>> redefine classes actually removes the breakpoints before 
>>>>>>> updating them (so there is no breakpoints to update).
>>>>>>> We can just remove metadata_do.
>>>>>>>
>>>>>>>
>>>>>>> I also removed some unused code.
>>>>>>>
>>>>>>> Changeset:
>>>>>>> http://cr.openjdk.java.net/~rehn/8235912/v1/webrev/
>>>>>>>
>>>>>>> Passes several runs of nsk jvmti/jdi and t1-7.
>>>>>>>
>>>>>>> Thanks, Robbin
>>>>>>
>>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191217/7d3286d2/attachment-0001.htm>