Re: RFR: 8231289: Disentangle JvmtiRawMonitor from ObjectMonitor and clean it up

[serguei.spit...@oracle.com]

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"); 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). It would be good to also run the jdk com/sun/jdi tests. The jdwp agent library is using the JVMTI RawMonitor's. 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 -----