RFR: 8231289: Disentangle JvmtiRawMonitor from ObjectMonitor and clean it up
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Oct 1 08:06:32 UTC 2019
I've started reviewing this and expecting to finish it tomorrow.
On 9/30/19 15:52, David Holmes wrote:
> 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:
>> 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.
>> - 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.
More information about the hotspot-runtime-dev