RFC: 8229160: Reimplement JvmtiRawMonitor to use PlatformMonitor

[David Holmes]

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 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 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 
>>
>