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

[Daniel D. Daugherty]

Sorry for the delay in reviewing this one... I've been playing whack-a-mole
with Robbin's MoCrazy test and my AsyncMonitorDeflation bits...


On 9/24/19 1:09 AM, David Holmes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8231289
> webrev: http://cr.openjdk.java.net/~dholmes/8231289/webrev/

src/hotspot/share/prims/jvmtiEnv.cpp
     Thanks for removing the PROPER_TRANSITIONS stuff. That was old
     and crufty stuff.

src/hotspot/share/prims/jvmtiEnvBase.cpp
     No comments.

src/hotspot/share/prims/jvmtiRawMonitor.cpp
     L39:   new (ResourceObj::C_HEAP, mtInternal) 
GrowableArray<JvmtiRawMonitor*>(1,true);
         nit - need a space between ',' and 'true'.

         Update: leave for your follow-up bug.

src/hotspot/share/prims/jvmtiRawMonitor.hpp
     No comments.

src/hotspot/share/runtime/objectMonitor.hpp
     Glad I added those 'protected for JvmtiRawMonitor' in one
     of my recent cleanup bugs. Obviously I'll have to merge
     with Async Monitor Deflation. :-)

src/hotspot/share/runtime/thread.cpp
     No comments.

src/hotspot/share/runtime/thread.hpp
     No comments.

src/hotspot/share/services/threadService.cpp
     L397:     waitingToLockMonitor = jt->current_pending_monitor();
     L398:     if (waitingToLockMonitor == NULL) {
     L399:       // we can only be blocked on a raw monitor if not 
blocked on an ObjectMonitor
     L400:       waitingToLockRawMonitor = 
jt->current_pending_raw_monitor();
     L401:     }

         JVM/TI has this event handler:

           typedef void (JNICALL *jvmtiEventMonitorContendedEnter)
               (jvmtiEnv *jvmti_env,
                JNIEnv* jni_env,
                jthread thread,
                jobject object);

         This event handler is called after set_current_pending_monitor()
         and if the event handler uses a RawMonitor, then it possible for
         for the thread to show up as blocked on both a Java monitor and
         a JVM/TI RawMonitor.

test/hotspot/jtreg/vmTestbase/nsk/jvmti/RawMonitorWait/rawmnwait005/rawmnwait005.cpp
     No comments.


Thumbs up! The only non-nit I have is the setting of waitingToLockRawMonitor
on L400 and the corresponding comment on L399. Everything else is a nit.

I don't need to see a new webrev.

Thanks for tackling this disentangle issue!

Dan


>
> 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
> -----