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 serviceability-dev
mailing list