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

[David Holmes]

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.

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

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

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