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

[serguei.spitsyn at oracle.com]

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");


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