RFR: 8231289: Disentangle JvmtiRawMonitor from ObjectMonitor and clean it up

[David Holmes]

Hi Dan,

Thanks for the second look.

On 5/10/2019 1:51 am, Daniel D. Daugherty wrote:
> On 10/3/19 11:58 PM, David Holmes wrote:
>> Okay, to allow for me to make forward progress here I've decided to 
>> follow the "principle of least brokenness" ;-)
>>
>> Recap: Because of JVMTI event callbacks it is possible for a thread to 
>> set its current pending monitor as a JvmtiRawMonitor when it was 
>> already set for an ObjectMonitor. This is broken in at least two ways:
>> - when the raw monitor use completes the pending monitor is set to 
>> NULL, not restored to the ObjectMonitor
>> - whilst the raw monitor is seen as the pending monitor the 
>> ObjectMonitor is not considered by the deadlock detection logic
>>
>> Ignoring what I'm currently doing with jvmtiRawMonitor, I do not know 
>> how to fix the above brokenness and it is out of scope for what I am 
>> trying to do.
>>
>> So what I propose is to make things no more broken than they are now, 
>> and actually improve things a little:
>> - the pending JvmtiRawMonitor is given preference over the 
>> ObjectMonitor in the deadlock detection code (this emulates current 
>> situation of the raw monitor overwriting the pending ObjectMonitor)
>> - we no longer lose the fact we were also pending on an ObjectMonitor
>> - the stack printing code in the deadlock detector prints information 
>> about both the raw monitor and the ObjectMonitor
>>
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8231289/webrev.v2/
> 
> src/hotspot/share/services/threadService.cpp
>      L399:     // waiting on both a raw monitor and ObjectMonitor at the 
> same time. It
>          s/waiting on/waiting to lock/

Fixed.

>      L981:      const char* owner_desc = ",\n  which is held by";
>      L994:            st->print_cr(",\n  which has now been released");
>      L1008:         owner_desc = "\n  in JNI, which is held by";
>          Not your bug for L981 or L1008, but those '\n' aren't portable 
> right?
>          I think a 'st->cr()' is needed instead.

\n within a string literal should be perfectly portable. It's a symbolic 
representation of a "newline". If it eventually get written to somewhere 
that cares (like a file) it will be converted into appropriate CR or 
CRLF characters for the platform.

>          Of course, I don't understand why the newline is there anyway,
>          but without seeing an example output its hard to say.

Yes I assume it looks pretty if you see it written out.

> Thumbs up. Don't need to see another webrev...

Thanks,
David
-----

> Dan
> 
> 
>>
>> The only change is threadService.cpp
>>
>> Thanks,
>> David
>