RFR 8247808: Move JVMTI strong oops to OopStorage

[Stefan Karlsson]

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.

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.

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

StefanK

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