Sorry for the delay in reviewing this one... I've been playing whack-a-mole
with Robbin's MoCrazy test and my AsyncMonitorDeflation bits...
On 9/24/19 1:09 AM, David Holmes wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8231289
webrev: http://cr.openjdk.java.net/~dholmes/8231289/webrev/
src/hotspot/share/prims/jvmtiEnv.cpp
Thanks for removing the PROPER_TRANSITIONS stuff. That was old
and crufty stuff.
src/hotspot/share/prims/jvmtiEnvBase.cpp
No comments.
src/hotspot/share/prims/jvmtiRawMonitor.cpp
L39: new (ResourceObj::C_HEAP, mtInternal)
GrowableArray<JvmtiRawMonitor*>(1,true);
nit - need a space between ',' and 'true'.
Update: leave for your follow-up bug.
src/hotspot/share/prims/jvmtiRawMonitor.hpp
No comments.
src/hotspot/share/runtime/objectMonitor.hpp
Glad I added those 'protected for JvmtiRawMonitor' in one
of my recent cleanup bugs. Obviously I'll have to merge
with Async Monitor Deflation. :-)
src/hotspot/share/runtime/thread.cpp
No comments.
src/hotspot/share/runtime/thread.hpp
No comments.
src/hotspot/share/services/threadService.cpp
L397: waitingToLockMonitor = jt->current_pending_monitor();
L398: if (waitingToLockMonitor == NULL) {
L399: // we can only be blocked on a raw monitor if not
blocked on an ObjectMonitor
L400: waitingToLockRawMonitor =
jt->current_pending_raw_monitor();
L401: }
JVM/TI has this event handler:
typedef void (JNICALL *jvmtiEventMonitorContendedEnter)
(jvmtiEnv *jvmti_env,
JNIEnv* jni_env,
jthread thread,
jobject object);
This event handler is called after set_current_pending_monitor()
and if the event handler uses a RawMonitor, then it possible for
for the thread to show up as blocked on both a Java monitor and
a JVM/TI RawMonitor.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/RawMonitorWait/rawmnwait005/rawmnwait005.cpp
No comments.
Thumbs up! The only non-nit I have is the setting of waitingToLockRawMonitor
on L400 and the corresponding comment on L399. Everything else is a nit.
I don't need to see a new webrev.
Thanks for tackling this disentangle issue!
Dan
The earlier attempt to rewrite JvmtiRawMonitor as a simple wrapper
around PlatformMonitor proved not so simple and ultimately had too
many issues due to the need to support Thread.interrupt.
I'd previously stated in the bug report:
"In the worst-case I suppose we could just copy ObjectMonitor to a new
class and have JvmtiRawMonitor continue to extend that (with some
additional minor adjustments) - or even just inline it all as needed."
but hadn't looked at it in detail. Richard Reingruber did look at it
and pointed out that it is actually quite simple - we barely use any
actual code from ObjectMonitor, mainly just the state. So thanks
Richard! :)
So this change basically copies or moves anything needed by
JvmtiRawMonitor from ObjectMonitor, breaking the connection between
the two. We also copy and simplify ObjectWaiter, turning it into a
QNode internal class. There is then a lot of cleanup that was applied
(and a lot more that could still be done):
- Removed the never implemented/used PROPER_TRANSITIONS ifdefs
- Fixed the disconnect between the types of non-JavaThreads expected
by the upper layer code and lower layer code
- cleaned up and simplified return codes
- consolidated code that is identical for JavaThreads and
non-JavaThreads (e.g. notify/notifyAll).
- removed used of TRAPS/THREAD where not appropriate and replaced with
"Thread * Self" in the style of the rest of the code
- changed recursions to be int rather than intptr_t (a "fixme" in the
ObjectMonitor code)
I have not changed the many style flaws with this code:
- Capitalized names
- extra spaces before ;
- ...
but could do so if needed. I wanted to try and keep it more obvious
that the fundamental functional code is actually unmodified.
There is one aspect that requires further explanation: the notion of
current pending monitor. The "current pending monitor" is stored in
the Thread and used by a number of introspection APIs for things like
finding monitors, doing deadlock detection, etc. The JvmtiRawMonitor
code would also set/clear itself as "current pending monitor". Most
uses of the current pending monitor actually, explicitly or
implicitly, ignore the case when the monitor is a JvmtiRawMonitor
(observed by the fact the mon->object() query returns NULL). The
exception to that is deadlock detection where raw monitors are at
least partially accounted for. To preserve that I added the notion of
"current pending raw monitor" and updated the deadlock detection code
to use that.
The test:
test/hotspot/jtreg/vmTestbase/nsk/jvmti/RawMonitorWait/rawmnwait005/rawmnwait005.cpp
was updated because I'd noticed previously that it was the only test
that used interrupt with raw monitors, but was in fact broken: the
test thread is a daemon thread so the main thread could terminate the
VM immediately after the interrupt() call, thus you would never know
if the interruption actually worked as expected.
Testing:
- tiers 1 - 3
- vmTestbase/nsk/monitoring/ (for deadlock detection**)
- vmTestbase/nsk/jdwp
- vmTestbase/nsk/jdb/
- vmTestbase/nsk/jdi/
- vmTestbase/nsk/jvmti/
- serviceability/jvmti/
- serviceability/jdwp
- JDK: java/lang/management
** There are no existing deadlock related tests involving
JvmtiRawMonitor. It would be interesting/useful to add them to the
existing nsk/monitoring tests that cover synchronized and JNI locking.
But it's a non-trivial enhancement that I don't really have time to do.
Thanks,
David
-----
