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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Oct 4 00:01:34 UTC 2019


On 10/3/19 7:35 PM, serguei.spitsyn at oracle.com wrote:
>
>
> On 10/3/19 3:33 PM, David Holmes wrote:
>> On 4/10/2019 3:15 am, serguei.spitsyn at oracle.com wrote:
>>> On 10/3/19 03:13, David Holmes wrote:
>>>> Hi Dan,
>>>>
>>>> On 3/10/2019 3:20 am, Daniel D. Daugherty wrote:
>>>>> Sorry for the delay in reviewing this one... I've been playing 
>>>>> whack-a-mole
>>>>> with Robbin's MoCrazy test and my AsyncMonitorDeflation bits...
>>>>
>>>> No problem - your contribution made the wait worthwhile :)
>>>>
>>>>> 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.
>>>>
>>>> Fixed now so I don't forget later. :)
>>>>
>>>>> 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.
>>>>
>>>> Oh that is interesting - good catch! So that means the current code 
>>>> is broken because the raw monitor will replace the ObjectMonitor as 
>>>> the pending monitor and then set it back to NULL, thus losing the 
>>>> fact the thread is actually pending on the ObjectMonitor. And of 
>>>> course while the pending monitor is the raw monitor that totally 
>>>> messes up the deadlock detection as the ObjectMonitor is missing 
>>>> from consideration. :(
>>>
>>> If I remember correctly it is a scenario where this issue also comes 
>>> to the picture:
>>> https://bugs.openjdk.java.net/browse/JDK-8033399
>>>
>>> I do not really understand how shared ParkEvent can be used/consumed 
>>> by both ObjectMonitor and RawMonitor on the same thread.
>>> But we observed and investigated this problem several years ago and 
>>> Dan finally filed this enhancement.
>>
>> I still don't see how this is possible as you are not actually 
>> enqueued on the ObjectMonitor when the call out to the event callback 
>> is made. but that is a topic for another email thread. :)

Correct that you cannot be enqueued on the ObjectMonitor when you
make the callback. However, I don't think that was the point of
8033399 when we filed so very long ago...

Quoting a comment from David:

> David Holmes added a comment - 2014-01-27 18:34
> Could there be multiple places in event handling code that could in 
> theory consume unparks and so require the re-issue of an unpark() from 
> different locations in the code?
>
> Seems to me that perhaps raw monitors - given they can be entered from 
> within the normal monitor code - should have their own _event object 
> per thread, so that this accidental consumption of unparks can not occur. 


The scenario that comes to mind:

- T1 is contending on an ObjectMonitor and has set waitingToLockMonitor.
- T1 calls the jvmtiEventMonitorContendedEnter event handler that
   contends on a JvmtiRawMonitor and has set waitingToLockRawMonitor.
- T1 blocks on the JvmtiRawMonitor and parks.
- T2 is exiting the ObjectMonitor and has picked T1 as the successor
   so it unparks T1. Only T1 is parked for the JvmtiRawMonitor and
   not for the ObjectMonitor. T2 hasn't quite finished exiting the
   ObjectMonitor yet... (not sure if this lag is possible)
- T1 has unparked early for the JvmtiRawMonitor and at the same
   time T3 is exiting the JvmtiRawMonitor.
- T1 quickly enters the JvmtiRawMonitor and T3 doesn't have to
   pick a successor so it doesn't do an unpark.
- T1 finishes the work of the jvmtiEventMonitorContendedEnter and
   returns to the caller which is the code that's about to block on
   the ObjectMonitor...
- T1 blocks on the ObjectMonitor and parks.
- T2 finishes exiting the ObjectMonitor... Does T1 get unparked?

I can't remember when T2 does the unpark() relative to dropping
ownership of the ObjectMonitor. If the unpark() is first or if
the _owner field setting to NULL lingers... it's possible for T1
to block and park... with nothing to unpark T1...

Pure, crazy theory here...

However, with David's work on this bug (8231289), this theoretical
problem goes away... That's the only reason for trying to close
this 8033399 sub-thread here...

Dan


>
> Agreed.
> Just wanted to point out it can be related.
> Dan filed this RFE and can have more knowledge.
>
>> Meanwhile what to do about broken deadlock detection ... :(
>
> It is a good catch from Dan.
>
> Thanks,
> Serguei
>
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>>
>>>> This also probably means that you can have a pending raw monitor at 
>>>> the same time as you have a "Blocker" as I'm pretty sure there are 
>>>> various JVM TI event handlers that may execute between the Blocker 
>>>> being set and the actual park. So that would be an additional 
>>>> breakage in the existing code.
>>>>
>>>> Back to my code and I have two problems. The second, which is easy 
>>>> to address, is the deadlock printing code. I'll hoist the 
>>>> waitingToLockRawMonitor chunk to the top so it is executed 
>>>> independent of the waitingToLockMonitor value (which remains in an 
>>>> if/else relationship with the waitingToLockBlocker). But now that 
>>>> we might print two "records" at a time I have to make additional 
>>>> changes to get meaningful output for the current thread (which is 
>>>> handled as a common code after the if/else block to finish 
>>>> whichever record was being printed). Also I can no longer use 
>>>> "continue" as the 3 outcomes are not mutually exclusive - so this 
>>>> could get a bit messy. :(
>>>>
>>>> So definitely a v2 webrev on the way.
>>>>
>>>> But before that I need to solve my first problem - and I don't know 
>>>> how. Now that it is apparent that a thread can be blocked on both a 
>>>> raw monitor and an ObjectMonitor at the same time, I have no idea 
>>>> how to actually account for this in the deadlock detection code. 
>>>> That code has a while loop that expects to at most find either a 
>>>> locked ObjectMonitor or j.u.c Blocker, and it adds the owner thread 
>>>> to the cycle detection, then moves on. But now I can have two 
>>>> different owner threads in the same loop iteration. I don't know 
>>>> how to account for that.
>>>>
>>>> Given that it seems to me that the current code is already broken 
>>>> if we encounter these conditions, then perhaps all I can do is 
>>>> handle the other cases, where the blocking reasons are mutually 
>>>> exclusive, and not try to fix things? i.e. leave lines #434 to #440 
>>>> as they are in webrev v1 - which implies no change to line #398 
>>>> except the comment ... ??
>>>>
>>>>> 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.
>>>>
>>>> If only that were true :(
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> 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
>>>>>> -----
>>>>>
>>>
>



More information about the hotspot-runtime-dev mailing list