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

David Holmes david.holmes at oracle.com
Wed Oct 2 02:05:40 UTC 2019 

On 2/10/2019 9:27 am, coleen.phillimore at oracle.com wrote:
> On 10/1/19 6:21 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> Thanks for taking a look.
>>
>> On 2/10/2019 7:04 am, coleen.phillimore at oracle.com wrote:
>>>
>>> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html 
>>>
>>>
>>> I think it's odd that PROPER_TRANSITIONS looked like the right thing 
>>> to do, but we always took the alternate path with the large comment 
>>> about why it is evil.  Can we have an RFE to move this special 
>>> transition to interfaceSupport.inline.hpp as a supported transition, 
>>> so it is with the other ones? So we don't miss it if we changed more 
>>> things in the transitions. 
>>
>> I suppose we could - but I honestly don't think it is worth the 
>> effort. Once this is disentangled from ObjectMonitor we can blissfully 
>> ignore it and should never need to change it. The transition from one 
>> safepoint-safe state to another is safe as the comment notes, and not 
>> really "evil" IMO. IIRC we make the same observation elsewhere about 
>> moving between safe states.
> 
> I wonder how many other places in the VM we've sprinkled around the code 
> to change thread states.

There are a few special cases

- debug stubs in macroassemblers
- native method calls from interpreter/JIT
- JFR emergency dump code
- thread initialization/startup
- thread self-suspension
- VM exit
- some weird JVMTI event proxying logic I really don't follow :)

and of course safepoint blocking itself.

>>
>>> Should this transition check for NoSafepointVerifier, for example?
>>
>> I have no idea what the NSV would mean in this context. JavaThreads 
>> using raw monitors have no interaction with safepoints whilst using 
>> them. The thread is always in a safepoint-safe state and so ignored by 
>> the safepoint logic. We don't care how many safepoints might occur 
>> whilst executing this code.
> 
> You're right, we're changing from native to blocked so yes, it doesn't 
> need to check that safepoints are ok.
>>
>>> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/runtime/objectMonitor.hpp.frames.html 
>>>
>>>
>>>   39 // ParkEvent instead.  Beware, however, that the JVMTI code
>>>    40 // knows about ObjectWaiters, so we'll have to reconcile that 
>>> code.
>>>    41 // See next_waiter(), first_waiter(), etc.
>>>
>>> Comment should be removed.
>>
>> No that is about JVMTI, not JVMTI raw monitors. JVMTI uses 
>> ObjectWaiter for reporting on monitor usage.
> 
> That's really confusing.
>>
>>> http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/prims/jvmtiRawMonitor.cpp.frames.html 
>>>
>>>
>>> 148 void JvmtiRawMonitor::SimpleExit (Thread * Self) {
>>>
>>> 128 QNode Node (Self) ;
>>>
>>> 153 QNode * w ;
>>>
>>> 162 guarantee (w ->TState == QNode::TS_ENTER, "invariant") ;
>>>
>>>
>>> Can you fix the style problems in the lines you changed, like these? 
>>> Fixing the style in _changed_ code doesn't distract from the code 
>>> review (having the old style does!)
>>
>> I'm happy to do a full style cleanup on this code later, but mixing 
>> old and new styles just seems more distracting to me. I emulate the 
>> existing style on the lines I change.
> 
> Do you have an RFE to clean this up?

I do now :)

https://bugs.openjdk.java.net/browse/JDK-8231737

Thanks,
David


> thanks,
> Coleen
>>
>>> This looks like a nice cleanup, and does not appear to change the 
>>> what the code does, but someone who understands the locking code 
>>> should also review it.
>>
>> Thanks again for taking a look.
>>
>> David
>> -----
>>
>>> Thanks,
>>> Coleen
>>>
>>> On 9/30/19 6:52 PM, David Holmes wrote:
>>>> ping!
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 24/09/2019 3:09 pm, 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