On 6/18/20 3:25 AM, Stefan Karlsson wrote:
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.
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
