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

[David Holmes]

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