RFC: 8229160: Reimplement JvmtiRawMonitor to use PlatformMonitor

[David Holmes]

Hi Serguei,

On 21/08/2019 9:58 am, serguei.spitsyn at oracle.com wrote:
> Hi David,
> 
> The whole approach looks good to me.

Thanks for taking a look. My main concern is about the interrupt 
semantics, so I really need to get some end-user feedback on that aspect 
as well.

> + if (jSelf != NULL) {
> + if (interruptible && Thread::is_interrupted(jSelf, true)) {
> + // We're now interrupted but we may have consumed a notification.
> + // To avoid lost wakeups we have to re-issue that notification, which
> + // may result in a spurious wakeup for another thread. Alternatively we
> + // ignore checking for interruption before returning.
> + notify();
> + return false; // interrupted
> + }
> 
> I'm a bit concerned about introduction of new spurious wake ups above.
> Some tests can be not defensive against it, so we may discover new 
> intermittent failures.

That is possible. Though given spurious wakeups are already possible any 
test that is incorrectly using RawMonitorWait() without checking a 
condition, is technically already broken.

Not checking for interruption after the wait will also require some test 
changes, and it weakens the interrupt semantics even further.

Thanks,
David
-----

> Thanks,
> Serguei
> 
> On 8/14/19 11:22 PM, 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
>