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

[serguei.spitsyn at oracle.com]

Hi David,

I've started reviewing this and expecting to finish it tomorrow.

Thanks,
Serguei


On 9/30/19 15:52, David Holmes wrote:
> 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
>> -----