RFC: 8229160: Reimplement JvmtiRawMonitor to use PlatformMonitor

[Daniel D. Daugherty]

On 9/8/19 10:15 PM, David Holmes wrote:
> Hi Dan,
>
> On 7/09/2019 6:50 am, Daniel D. Daugherty wrote:
>> Hi David,
>>
>> I've finally gotten back to this email thread...
>
> Thanks.
>
>>> FYI testing to date:
>>>  - tiers 1 -3 all platforms
>>>  - hotspot: serviceability/jvmti
>>>                           /jdwp
>>>             vmTestbase/nsk/jvmti
>>>                           /jdwp
>>>  - JDK: com/sun/jdi 
>>
>> You should also add:
>>
>> open/test/hotspot/jtreg/vmTestbase/nsk/jdb
>> open/test/hotspot/jtreg/vmTestbase/nsk/jdi
>> open/test/jdk/java/lang/instrument
>
> Okay - in progress. Though I can't see any use of RawMonitors in any 
> of these tests.

I wouldn't expect direct use. I would expect built-on-top-of use. In
particular, there are scenario tests in ndk/jdi that use JDI in different
ways than straight up API tests and shake out bugs.


>
>> I took a quick look through the preliminary webrev and I don't see
>> anything that worries me.
>
> Thanks. I'll prepare a more polished webrev soon.
>
>> Re: Thread.interrupt() and raw_wait()
>>
>> It would be good to see if that semantic is being tested via the
>> JCK test suite for JVM/TI.
>
> It isn't. The only thing directly tested for RawMonitorWait is normal 
> successful operation and reporting "not owner" when not the owner. No 
> check for JVMTI_ERROR_INTERRUPT exists other than as input for the 
> GetErrorName function.
>
> There's only one test in the whole test base that checks for the 
> interrupt and that is 
> vmTestbase/nsk/jvmti/RawMonitorWait/rawmnwait005/. In that test if we 
> are not interrupted before the RawMonitorWait we will wait until the 
> full timeout elapses - which is 2 minutes by default - then return and 
> report the interrupt. Hence the test still passes. (If it was an 
> untimed wait that would be different of course).
>
> The more I try to convince people this change should be okay, the more 
> uncomfortable I get with my own arguments. :) I think I'm going to 
> implement the polling approach for checking interrupts - say 500ms.

I'll keep an eye open for the update...

Dan

>
>> I also very much like/appreciate the decoupling of JvmtiRawMonitors
>> from ObjectMonitors... Thanks for tackling this crazy task.
>
> Thanks :)
>
> David
>
>> Dan
>>
>>
>>
>> On 8/15/19 2:22 AM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8229160
>>>
>>> Preliminary webrev (still has rough edges): 
>>> http://cr.openjdk.java.net/~dholmes/8229160/webrev.prelim/
>>>
>>> Background:
>>>
>>> We've had this comment for a long time:
>>>
>>>  // The raw monitor subsystem is entirely distinct from normal
>>>  // java-synchronization or jni-synchronization.  raw monitors are not
>>>  // associated with objects.  They can be implemented in any manner
>>>  // that makes sense.  The original implementors decided to piggy-back
>>>  // the raw-monitor implementation on the existing Java 
>>> objectMonitor mechanism.
>>>  // This flaw needs to fixed.  We should reimplement raw monitors as 
>>> sui-generis.
>>>  // Specifically, we should not implement raw monitors via java 
>>> monitors.
>>>  // Time permitting, we should disentangle and deconvolve the two 
>>> implementations
>>>  // and move the resulting raw monitor implementation over to the 
>>> JVMTI directories.
>>>  // Ideally, the raw monitor implementation would be built on top of
>>>  // park-unpark and nothing else.
>>>
>>> This is an attempt to do that disentangling so that we can then 
>>> consider changes to ObjectMonitor without having to worry about 
>>> JvmtiRawMonitors. But rather than building on low-level park/unpark 
>>> (which would require the same manual queue management and much of 
>>> the same complex code as exists in ObjectMonitor) I decided to try 
>>> and do this on top of PlatformMonitor.
>>>
>>> The reason this is just a RFC rather than RFR is that I overlooked a 
>>> non-trivial aspect of JvmtiRawMonitors: like Java monitors (as 
>>> implemented by ObjectMonitor) they interact with the 
>>> Thread.interrupt mechanism. This is not clearly stated in the JVM TI 
>>> specification [1] but only in passing by the possible errors for 
>>> RawMonitorWait:
>>>
>>> JVMTI_ERROR_INTERRUPT    Wait was interrupted, try again
>>>
>>> As I explain in the bug report there is no way to build in proper 
>>> interrupt support using PlatformMonitor as there is no way we can 
>>> "interrupt" the low-level pthread_cond_wait. But we can approximate 
>>> it. What I've done in this preliminary version is just check 
>>> interrupt state before and after the actual "wait" but we won't get 
>>> woken by the interrupt once we have actually blocked. Alternatively 
>>> we could use a periodic polling approach and wakeup every Nms to 
>>> check for interruption.
>>>
>>> The only use of JvmtiRawMonitors in the JDK libraries (JDWP) is not 
>>> affected by this choice as that code ignores the interrupt until the 
>>> real action it was waiting for has occurred. The interrupt is then 
>>> reposted later.
>>>
>>> But more generally there could be users of JvmtiRawMonitors that 
>>> expect/require that RawMonitorWait is responsive to Thread.interrupt 
>>> in a manner similar to Object.wait. And if any of them are reading 
>>> this then I'd like to know - hence this RFC :)
>>>
>>> FYI testing to date:
>>>  - tiers 1 -3 all platforms
>>>  - hotspot: serviceability/jvmti
>>>                           /jdwp
>>>             vmTestbase/nsk/jvmti
>>>                           /jdwp
>>>  - JDK: com/sun/jdi
>>>
>>> Comments/opinions appreciated.
>>>
>>> Thanks,
>>> David
>>>
>>> [1] 
>>> https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#RawMonitorWait 
>>>
>>