Hi David, I think this change makes sense. We'll test it and take a closer look at it. First impression is good.
Best regards, Martin > -----Original Message----- > From: David Holmes <[email protected]> > Sent: Dienstag, 27. August 2019 00:21 > To: [email protected]; serviceability-dev <serviceability- > [email protected]>; [email protected]; [email protected]; > [email protected]; Doerr, Martin <[email protected]> > Subject: Re: RFC: 8229160: Reimplement JvmtiRawMonitor to use > PlatformMonitor > > On 26/08/2019 6:25 pm, [email protected] wrote: > > Hi David, > > > > > > On 8/20/19 22:21, David Holmes wrote: > >> Hi Serguei, > >> > >> On 21/08/2019 9:58 am, [email protected] 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 > >>>> > >>> > >
