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

[David Holmes]

ping!

Thanks,
David

On 24/09/2019 3:09 pm, 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
> -----