I forgot to say, the fix looks pretty
good to me.
Also, it is quite educational. :)
On 10/2/19 01:51, [email protected] wrote:
Hi David,
http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/services/threadService.cpp.frames.html
Minor comment:
397 waitingToLockMonitor = jt->current_pending_monitor();
398 if (waitingToLockMonitor == NULL) {
399 // we can only be blocked on a raw monitor if not blocked on an ObjectMonitor
400 waitingToLockRawMonitor = jt->current_pending_raw_monitor();
401 }
402 if (concurrent_locks) {
403 waitingToLockBlocker = jt->current_park_blocker();
404 }
If I understand correctly, a thread can wait to lock only one of
the three locks.
So, we could rewrite the line 402 as:
if (concurrent_locks &&
waitingToLockRawMonitor == NULL) {
But I do not care much about this pre-existed logic.
Maybe adding an assert after the line 404 would make sense:
assert( waitingToLockRawMonitor == NULL
|| waitingToLockBlocker == NULL,
"invariant");
This assert above can be enhanced:
assert(waitingToLockMonitor == NULL || waitingToLockRawMonitor == NULL
|| waitingToLockBlocker == NULL, "invariant");
Thanks,
Serguei
Bug:
https://bugs.openjdk.java.net/browse/JDK-8231289
webrev: http://cr.openjdk.java.net/~dholmes/8231289/webrev/
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
-----
|
