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

[coleen.phillimore at oracle.com]

On 12/16/19 8:13 AM, Robbin Ehn wrote:
> Hi Coleen, in VM_RedefineClasses::doit:
>
> This updates the breakpoints:
>   MetadataOnStackMark md_on_stack(/*walk_all_metadata*/true, 
> /*redefinition_walk*/true);
>
> And this removes breakpoints:
>   for (int i = 0; i < _class_count; i++) {
>     redefine_single_class(_class_defs[i].klass, _scratch_classes[i], 
> thread);
>   }
>
> So we skip updating, since we do remove them after we updated them.
> But you are the expert here. Let me know if there is something I missed.
>

No, you are correct. The JVMTI spec says that the breakpoints are all 
deleted.  I'm remembering code that sets/clears breakpoints that has to 
walk emcp methods, and set them there also.  But redefinition does clear 
them.

If the old Method* is still executing or referenced somehow, the other 
metadata walking would find it anyway.  So maybe this was never needed.

> OopHandle just adds more code.
>

It doesn't.  And if we want to make all native memory never point 
directly to oops and point to oopStorage instead, having some 
encapsulation makes that easier.  It also makes it so that we don't have 
to stare at oop* in data structures and wonder if we're going to miss 
the mumble-fratz access and decorators that we need.  ie:

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


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,
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/20191216/42d626c9/attachment-0001.htm>