RFC: 8229160: Reimplement JvmtiRawMonitor to use PlatformMonitor

Wed Aug 28 10:53:42 UTC 2019

Hi David,

I've run it through our nightly tests and got an assertion on Windows.

Test:
vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t001

Assertion:
#  Internal Error (src/hotspot/share/prims/jvmtiRawMonitor.cpp:173), pid=17824, tid=7808
#  assert(__the_thread__->is_VM_thread()) failed: must be VM thread

Current thread (0x00000213a5dbb800):  GCTaskThread "GC Thread#0" [stack: 0x0000001bfbe00000,0x0000001bfbf00000] [id=7808]

Stack: [0x0000001bfbe00000,0x0000001bfbf00000]
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
...
V  [jvm.dll+0x50d1a2]  report_vm_error+0x102  (debug.cpp:264)
V  [jvm.dll+0x8f3996]  JvmtiRawMonitor::raw_enter+0x1e6  (jvmtirawmonitor.cpp:173)
V  [jvm.dll+0x8d6121]  JvmtiEnv::RawMonitorEnter+0x211  (jvmtienv.cpp:3345)
C  [ap04t001.dll+0x30ef]

So this assumption is not true:
     // No other non-Java threads besides VM thread would acquire
     // a raw monitor.

This is the only issue I've seen so far.

Best regards,
Martin

> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Dienstag, 27. August 2019 00:21
> To: serguei.spitsyn at oracle.com; serviceability-dev <serviceability-
> dev at openjdk.java.net>; jcbeyler at google.com; yasuenag at gmail.com;
> sgehwolf at redhat.com; Doerr, Martin <martin.doerr at sap.com>
> Subject: Re: RFC: 8229160: Reimplement JvmtiRawMonitor to use
> PlatformMonitor
> 
> On 26/08/2019 6:25 pm, serguei.spitsyn at oracle.com wrote:
> > Hi David,
> >
> >
> > On 8/20/19 22:21, David Holmes wrote:
> >> 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.
> >
> >
> > I don't have any opinion yet on what interrupt semantics tool developers
> > really need.
> > Yes, we may need to request some feedback.
> 
> I've now explicitly added JC, Yasumasa, Severin and Martin, to this
> email thread to try and solicit feedback from all the major players that
> seem interested in this serviceability area. Folks I'd really appreciate
> any feedback you may have here on the usecases for JvmtiRawMonitors,
> and
> in particular the use RawMonitorWait and its interaction with
> Thread.interrupt.
> 
> > My gut feeling tells me it is not good to break the original semantics. :)
> > But let me think about it a little bit more.
> 
> Me too, but I wanted to start simple. I suspect I will have to at least
> implement time-based polling of the interrupt state.
> 
> > Also, we need to file a CSR for this.
> 
> Depending on how this proceeds, yes.
> 
> >
> >>> + 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.
> >
> > Agreed.
> > I even think it is even better if spurious wakeups will happen more
> > frequently.
> > It should help to identify and fix such spots in the test base.
> 
> Yes it is good tests. Alas not so good for production code :)
> 
> >>
> >> Not checking for interruption after the wait will also require some
> >> test changes, and it weakens the interrupt semantics even further.
> >
> > I'm thinking about a small investigation on how this is used in our tests.
> 
> There seem to be a few uses that are susceptible to spurious wakeup
> errors, but those tests don't use interrupt.
> 
> Thanks,
> David
> 
> > Thanks,
> > Serguei
> >
> >>
> >> 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#RawMo
> nitorWait
> >>>>
> >>>
> >