RFC: 8229160: Reimplement JvmtiRawMonitor to use PlatformMonitor

[serguei.spitsyn at oracle.com]

Hi David,

The whole approach looks good to me.

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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190820/07b4895e/attachment.html>