RFR: 8203817: Monitor::try_lock() should not call check_prelock_state()

[Robbin Ehn]

>> http://cr.openjdk.java.net/~pliden/8203817/webrev.1

Thank you Per for this awesome patch, it solves a lot of special cases for me!

Ship it!

/Robbin

> 
> That seems a reasonable compromise.
> 
> There may still be possible deadlock issues if we use try-lock to acquire 
> multiple locks, but we probably don't do that ... and as Erik noted there are 
> issues with the ranking/deadlock detection anyway.
> 
> Thanks,
> David
> 
>> /Per
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Perhaps locking without safepont checks could perform more sanity checks, 
>>>> but that is a separate issue. I think try_lock should perform the same 
>>>> checks that locking without safepoint checks does. The alternatives are then to
>>>>
>>>> 1) Remove checking the prelock state like Per suggests for try_lock (then 
>>>> they do the same checks), or
>>>> 2) Overhaul the safepoint checking refactoring out the bits that check the 
>>>> safepointing sanity chrcka from other deadlock checks (and correct those to 
>>>> check for rank <= special, and not == special), remove the safepoint 
>>>> checking part from try_lock and adding the deadlock checking parts to lock 
>>>> without safepoints.
>>>>
>>>> Doing #2 seems like a different RFE. In fact I believe that would be 
>>>> https://bugs.openjdk.java.net/browse/JDK-8184732 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8184732?jql=text%20~%20%22deadlock%20check%22> that 
>>>> I filed a while back.
>>>>
>>>> In summary, there is a whole bunch of problems in the deadlock detection 
>>>> system, and #2 makes it hard to not get dragged down in the rabbit hole. #1 
>>>> is sufficient to  make try_lock check as much (or little) as locking without 
>>>> safepoint checking. And I think that is enough for the scope of this change.
>>>>
>>>> Looks good to me.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 25 May 2018, at 08:53, David Holmes <david.holmes at oracle.com 
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>>> Hi Per,
>>>>>
>>>>> Exactly what condition(s) does JFR violate? This is throwing away all the 
>>>>> checks that guard against incorrect monitor use. It's not just about 
>>>>> whether you'd block trying to acquire the Monitor, it's also about whether 
>>>>> it is safe to acquire it from that code/thread in the first place. (Though 
>>>>> I think some of the checks in there should also be considering the value of 
>>>>> _safepoint_check_required.)
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>
>>>>> On 25/05/2018 4:39 PM, Per Liden wrote:
>>>>>> In debug builds, Monitor::try_lock() calls check_prelock_state() to check 
>>>>>> the thread state, etc. The intention is to verify that the call is made 
>>>>>> from the correct context, a context where we're allowed to block and 
>>>>>> potentially safepoint. Unlike Monitor::lock(), Monitor::try_lock() will 
>>>>>> never block, hence the call to check_prelock_state() is overly strict and 
>>>>>> we should remove it. Removing it would match the behavior of all other 
>>>>>> non-blocking functions, like Monitor::lock_without_safepoint_check(), 
>>>>>> which doesn't call check_prelock_state() either (for a good reason).
>>>>>> The specific problem I've run into with this is related to JFR. 
>>>>>> Monitor::try_lock() is by JFR to allow non-blocking event generation, so 
>>>>>> that you can generate JFR events from "any" context without risk 
>>>>>> blocking/safepointing (the logic is doing something like, if try_lock() 
>>>>>> fails then put the event on a different queue and let the next owner of 
>>>>>> the lock handle it). The overly strict checks done by 
>>>>>> check_prelock_state() in try_lock() breaks this logic, which in turn means 
>>>>>> that you can't generate JFR event from "any" context as was intended.
>>>>>> The patch to fix this is a one-liner, just remove the call to 
>>>>>> check_prelock_state().
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203817
>>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8203817/webrev.0
>>>>>> /Per