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

[Per Liden]

Hi David,

On 05/25/2018 08:53 AM, David Holmes wrote:
> Hi Per,
> 
> Exactly what condition(s) does JFR violate? This is throwing away all 

I know of two issues:

This:

   assert((!thread->is_Java_thread() || ((JavaThread 
*)thread)->thread_state() == _thread_in_vm)
          || rank() == Mutex::special, "wrong thread state for using 
locks");

Because we're a Java thread, in a VM-leaf call context, grabbing a 
Mutex::leaf lock.

And this:

     debug_only(if (rank() != Mutex::special) \
                thread->check_for_valid_safepoint_state(false);)

Which ends up doing a similar check inside 
check_for_valid_safepoint_state().

/Per

> 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