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

[David Holmes]

Hi Coleen,

Thanks for taking a look.

On 2/10/2019 7:04 am, coleen.phillimore at oracle.com wrote:
> 
> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
> 
> I think it's odd that PROPER_TRANSITIONS looked like the right thing to 
> do, but we always took the alternate path with the large comment about 
> why it is evil.  Can we have an RFE to move this special transition to 
> interfaceSupport.inline.hpp as a supported transition, so it is with the 
> other ones? So we don't miss it if we changed more things in the 
> transitions. 

I suppose we could - but I honestly don't think it is worth the effort. 
Once this is disentangled from ObjectMonitor we can blissfully ignore it 
and should never need to change it. The transition from one 
safepoint-safe state to another is safe as the comment notes, and not 
really "evil" IMO. IIRC we make the same observation elsewhere about 
moving between safe states.

> Should this transition check for NoSafepointVerifier, for 
> example?

I have no idea what the NSV would mean in this context. JavaThreads 
using raw monitors have no interaction with safepoints whilst using 
them. The thread is always in a safepoint-safe state and so ignored by 
the safepoint logic. We don't care how many safepoints might occur 
whilst executing this code.

> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/runtime/objectMonitor.hpp.frames.html
> 
>   39 // ParkEvent instead.  Beware, however, that the JVMTI code
>    40 // knows about ObjectWaiters, so we'll have to reconcile that code.
>    41 // See next_waiter(), first_waiter(), etc.
> 
> Comment should be removed.

No that is about JVMTI, not JVMTI raw monitors. JVMTI uses ObjectWaiter 
for reporting on monitor usage.

> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/prims/jvmtiRawMonitor.cpp.frames.html
> 
> 148 void JvmtiRawMonitor::SimpleExit (Thread * Self) {
> 
> 128 QNode Node (Self) ;
> 
> 153 QNode * w ;
> 
> 162 guarantee (w ->TState == QNode::TS_ENTER, "invariant") ;
> 
> 
> Can you fix the style problems in the lines you changed, like these?  
> Fixing the style in _changed_ code doesn't distract from the code review 
> (having the old style does!)

I'm happy to do a full style cleanup on this code later, but mixing old 
and new styles just seems more distracting to me. I emulate the existing 
style on the lines I change.

> This looks like a nice cleanup, and does not appear to change the what 
> the code does, but someone who understands the locking code should also 
> review it.

Thanks again for taking a look.

David
-----

> Thanks,
> Coleen
> 
> On 9/30/19 6:52 PM, 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
>>> -----
>