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

[serguei.spitsyn at oracle.com]

Hi Coleen,

Is this webrev v2 right to look at? :
   http://cr.openjdk.java.net/~coleenp/2019/8235829.02/webrev/

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/83b609a7/attachment.htm>