RFR 8247808: Move JVMTI strong oops to OopStorage

[coleen.phillimore at oracle.com]

On 6/18/20 3:25 AM, Stefan Karlsson wrote:
> Hi Coleen,
>
> On 2020-06-17 23:25, coleen.phillimore at oracle.com wrote:
>> Summary: Remove JVMTI oops_do calls from JVMTI and GCs
>>
>> Tested with tier1-3, also built shenandoah to verify shenandoah changes.
>>
>> open webrev at 
>> http://cr.openjdk.java.net/~coleenp/2020/8247808.01/webrev
>
> https://cr.openjdk.java.net/~coleenp/2020/8247808.01/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.udiff.html 
>
>
>  JvmtiBreakpoint::~JvmtiBreakpoint() {
> - if (_class_holder != NULL) {
> - NativeAccess<>::oop_store(_class_holder, (oop)NULL);
> - OopStorageSet::vm_global()->release(_class_holder);
> + if (_class_holder.resolve() != NULL) {
> + _class_holder.release();
>    }
>  }
>
> Could this be changed to peek() / release() instead? The resolve() 
> call is going to keep the object alive until next for ZGC marking cycle.

Yes, makes sense. Fixed.
>
> The rest looks OK.
>
> Below are some comments about things that I find odd and non-obvious 
> from reading the code, and may be potentials for cleanups to make it 
> easier for the next to understand the code:
>
> The above code assumes that as soon as OopHandle::create has been 
> called, we won't store NULL into the _obj pointer. If someone does, 
> then we would leak the memory. OopHandle has a function ptr_raw, that 
> allows someone to clear the _obj pointer. I have to assume that this 
> function isn't used in this code.
>
> ---
>
>  214 void JvmtiBreakpoint::copy(JvmtiBreakpoint& bp) {
>  215   _method   = bp._method;
>  216   _bci      = bp._bci;
> 217 _class_holder = OopHandle::create(bp._class_holder.resolve());
>  218 }
>
> This one looks odd, because the _class_holder is overwritten without 
> releasing the old OopHandle. This is currently OK, because copy is 
> only called from clone, which just created a new JvmtiBreakpoint:
>
>   GrowableElement *clone()        {
>     JvmtiBreakpoint *bp = new JvmtiBreakpoint();
>     bp->copy(*this);
>     return bp;
>   }
>
>  I think this would have been much more obvious if copy/clone were a 
> copy constructor.

Yes, this would make more sense.  I don't know why this was implemented 
as clone.
>
> With that said, it looks like we now have two JvmtiBreakpoints with 
> the same OopHandle contents. So, OopHandle::release will be called 
> twice. Now that works because release clears the oop value:
>
> inline void OopHandle::release() {
>   // Clear the OopHandle first
>   NativeAccess<>::oop_store(_obj, (oop)NULL);
>   OopStorageSet::vm_global()->release(_obj);
> }
>
> and the resolve() != NULL check will prevent the OopHandle from being 
> released twice:
>
> + if (_class_holder.resolve() != NULL) {
> + _class_holder.release();
>    }

The release is called on the original JvmtiBreakpoint which has one 
OopHandle, and it's also called on the copy which has another, so 
release isn't called twice on the same OopHandle.

That said, I had to walk through the code this morning and make sure 
that release is called on the copy of the JvmtiBreakpoint (it's called 
in remove() after the breakpoint is cleared.  The entire _bps array is 
not deleted).

Thanks,
Coleen
>
> StefanK
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8247808
>>
>> Thanks,
>> Coleen
>