Hi Coleen,
On 2020-06-17 23:25, coleen.phillim...@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
