RFC: 8229160: Reimplement JvmtiRawMonitor to use PlatformMonitor

David Holmes david.holmes at oracle.com
Wed Aug 28 12:33:14 UTC 2019 

Hi Martin,

On 28/08/2019 8:53 pm, Doerr, Martin wrote:
> 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

That's very interesting - the assertion exists in the current code as well.

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

Now why would a GCTaskThread being executing code that accesses 
JvmtiRawMonitors? Are we in some kind of event callback?

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

Is there any more stack? What is that dll?

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

As I said this is not a new assertion so its interesting that you fund this.

Can you tell me what test this was and how to reproduce?

Thanks,
David
-----

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

More information about the serviceability-dev mailing list