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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Mon Oct 7 17:51:07 UTC 2019


Hi David,

Got it, thanks!
Serguei


On 10/7/19 04:33, David Holmes wrote:
> 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
>>>>>> -----
>>>>>
>>>>
>>



More information about the hotspot-runtime-dev mailing list