Hi Serguei,
One follow up ...
On 7/10/2019 5:45 pm, [email protected] wrote:
Hi David,
Thank you for replies!
On 10/3/19 18:59, David Holmes wrote:
Hi Serguei,
Just coming back to your original review emails to ensure everything
covered.
On 2/10/2019 6:57 pm, [email protected] wrote:
I forgot to say, the fix looks pretty good to me.
Also, it is quite educational. :)
Thanks :)
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.
We now know this is not true.
Right, thanks.
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");
Again we now know this is not a valid assert.
Right. But this one has to be valid:
assert((waitingToLockMonitor == NULL && waitingToLockRawMonitor ==
NULL) ||
waitingToLockBlocker == NULL, "invariant");
I don't quite follow what that is asserting. But I'm not certain that
the waitingToBlockLocker must be NULL if either of the other two is
non-NULL. Given there exists a "method entry" event it would seem
possible to enter a JVMTI callback for that event after setting the
object that "waitingToBlockLocker" returns - which is done in
LockSupport.park.
public static void park(Object blocker) {
Thread t = Thread.currentThread();
setBlocker(t, blocker);
U.park(false, 0L);
setBlocker(t, null);
}
David
-----
http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/runtime/thread.hpp.frames.html
801 ParkEvent * _ParkEvent; // for Object monitors and JVMTI raw
monitors
We have an enhancement about the ParkEvent shared between
ObjectMonitor's and RawMonitor's:
https://bugs.openjdk.java.net/browse/JDK-8033399
Just wanted to hear your quick opinion if this enhancement still
needs to be fixed.
I see you comment in the bug report but confused why this is not a
problem anymore.
We may want to discuss it separately (e.g in the bug report comments).
Discussed elsewhere.
It would be good to also run the jdk com/sun/jdi tests.
The jdwp agent library is using the JVMTI RawMonitor's.
Tests run - no issues.
Thank you for checking!
Thanks,
Serguei
Thanks,
David
Thanks,
Serguei
On 9/23/19 22:09, David Holmes wrote:
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
-----
