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

[coleen.phillimore at oracle.com]

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
>>>>
>>