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

[David Holmes]

Hi Serguei,

One follow up ...

On 7/10/2019 5:45 pm, serguei.spitsyn at oracle.com wrote:
> Hi David,
> 
> Thank you for replies!
> 
> 
> On 10/3/19 18:59, David Holmes wrote:
>> Hi Serguei,
>>
>> Just coming back to your original review emails to ensure everything 
>> covered.
>>
>> On 2/10/2019 6:57 pm, serguei.spitsyn at oracle.com wrote:
>>> I forgot to say, the fix looks pretty good to me.
>>> Also, it is quite educational. :)
>>
>> Thanks :)
>>
>>> On 10/2/19 01:51, serguei.spitsyn at oracle.com wrote:
>>>> Hi David,
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/services/threadService.cpp.frames.html 
>>>>
>>>>
>>>> Minor comment:
>>>> 397 waitingToLockMonitor = jt->current_pending_monitor();
>>>> 398 if (waitingToLockMonitor == NULL) {
>>>> 399 // we can only be blocked on a raw monitor if not blocked on an 
>>>> ObjectMonitor
>>>> 400 waitingToLockRawMonitor = jt->current_pending_raw_monitor();
>>>> 401 }
>>>>   402     if (concurrent_locks) {
>>>>   403       waitingToLockBlocker = jt->current_park_blocker();
>>>>   404     }
>>>> If I understand correctly, a thread can wait to lock only one of the 
>>>> three locks.
>>>> So, we could rewrite the line 402 as:
>>>>   if (concurrent_locks &&waitingToLockRawMonitor == NULL) {
>>>>
>>>> But I do not care much about this pre-existed logic.
>>
>> We now know this is not true.
> 
> Right, thanks.
> 
> 
>>>>
>>>> Maybe adding an assert after the line 404 would make sense:
>>>>   assert(waitingToLockRawMonitor == NULL || waitingToLockBlocker == 
>>>> NULL, "invariant");
>>>
>>> This assert above can be enhanced:
>>>    assert(waitingToLockMonitor == NULL || waitingToLockRawMonitor == 
>>> NULL
>>>                                        || waitingToLockBlocker == 
>>> NULL, "invariant");
>>
>> Again we now know this is not a valid assert.
> 
> Right. But this one has to be valid:
>    assert((waitingToLockMonitor == NULL && waitingToLockRawMonitor == 
> NULL) ||
>           waitingToLockBlocker == NULL, "invariant");

I don't quite follow what that is asserting. But I'm not certain that 
the waitingToBlockLocker must be NULL if either of the other two is 
non-NULL. Given there exists a "method entry" event it would seem 
possible to enter a JVMTI callback for that event after setting the 
object that "waitingToBlockLocker" returns - which is done in 
LockSupport.park.

    public static void park(Object blocker) {
         Thread t = Thread.currentThread();
         setBlocker(t, blocker);
         U.park(false, 0L);
         setBlocker(t, null);
     }

David
-----

> 
>>> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/runtime/thread.hpp.frames.html 
>>>
>>>> 801 ParkEvent * _ParkEvent; // for Object monitors and JVMTI raw 
>>>> monitors
>>>> We have an enhancement about the ParkEvent shared between 
>>>> ObjectMonitor's and RawMonitor's:
>>>> https://bugs.openjdk.java.net/browse/JDK-8033399
>>>>
>>>> Just wanted to hear your quick opinion if this enhancement still 
>>>> needs to be fixed.
>>>> I see you comment in the bug report but confused why this is not a 
>>>> problem anymore.
>>>> We may want to discuss it separately (e.g in the bug report comments).
>>
>> Discussed elsewhere.
>>>
>>>>
>>>> It would be good to also run the jdk com/sun/jdi tests.
>>>> The jdwp agent library is using the JVMTI RawMonitor's.
>>
>> Tests run - no issues.
> 
> Thank you for checking!
> 
> Thanks,
> Serguei
> 
>>
>> Thanks,
>> David
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 9/23/19 22:09, 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
>>>>> -----
>>>>
>>>
>