RFR: 8210832: Remove sneaky locking in class Monitor

[Patricio Chilano]

Hi Robbin,

Thanks for reviewing this! Removing the block_in_safepoint_check thread 
local attribute is a great idea, here is v02:

Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v02/webrev
Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v02/inc/webrev/

Running mach5 again.

Thanks,
Patricio

On 1/28/19 8:31 AM, Robbin Ehn wrote:
> Hi Patricio,
>
> Mostly looks good!
>
> block_at_safepoint is always called with block_in_safepoint_check = 
> true. (correct?)
> Changing that to a local state instead of global simplifies the code.
>
> So I'm suggesting something like below.
>
> Thanks, Robbin
>
> diff -r e65cc445234c 
> src/hotspot/share/runtime/interfaceSupport.inline.hpp
> --- a/src/hotspot/share/runtime/interfaceSupport.inline.hpp    Mon Jan 
> 28 13:10:15 2019 +0100
> +++ b/src/hotspot/share/runtime/interfaceSupport.inline.hpp    Mon Jan 
> 28 14:10:59 2019 +0100
> @@ -308,2 +308,1 @@
> -    thread->block_in_safepoint_check = false;
> -    SafepointMechanism::block_at_safepoint(thread);
> +    SafepointMechanism::callback_if_safepoint(thread);
> @@ -323,2 +322,1 @@
> -      SafepointMechanism::block_at_safepoint(_thread);
> -      _thread->block_in_safepoint_check = true;
> +      SafepointMechanism::callback_if_safepoint(_thread);
> @@ -335,2 +332,0 @@
> -    } else {
> -      _thread->block_in_safepoint_check = true;
> @@ -337,0 +334,1 @@
> +
> diff -r e65cc445234c src/hotspot/share/runtime/safepoint.cpp
> --- a/src/hotspot/share/runtime/safepoint.cpp    Mon Jan 28 13:10:15 
> 2019 +0100
> +++ b/src/hotspot/share/runtime/safepoint.cpp    Mon Jan 28 14:10:59 
> 2019 +0100
> @@ -795,1 +795,1 @@
> -void SafepointSynchronize::block(JavaThread *thread) {
> +void SafepointSynchronize::block(JavaThread *thread, bool 
> block_in_safepoint_check) {
> @@ -850,1 +850,1 @@
> -      if (thread->block_in_safepoint_check) {
> +      if (block_in_safepoint_check) {
> @@ -880,1 +880,1 @@
> -          thread->block_in_safepoint_check) {
> +          block_in_safepoint_check) {
> diff -r e65cc445234c src/hotspot/share/runtime/safepoint.hpp
> --- a/src/hotspot/share/runtime/safepoint.hpp    Mon Jan 28 13:10:15 
> 2019 +0100
> +++ b/src/hotspot/share/runtime/safepoint.hpp    Mon Jan 28 14:10:59 
> 2019 +0100
> @@ -146,1 +146,1 @@
> -  static void   block(JavaThread *thread);
> +  static void   block(JavaThread *thread, bool 
> block_in_safepoint_check = true);
> diff -r e65cc445234c src/hotspot/share/runtime/safepointMechanism.hpp
> --- a/src/hotspot/share/runtime/safepointMechanism.hpp    Mon Jan 28 
> 13:10:15 2019 +0100
> +++ b/src/hotspot/share/runtime/safepointMechanism.hpp    Mon Jan 28 
> 14:10:59 2019 +0100
> @@ -82,1 +82,1 @@
> -  static inline void block_at_safepoint(JavaThread* thread);
> +  static inline void callback_if_safepoint(JavaThread* thread);
> diff -r e65cc445234c 
> src/hotspot/share/runtime/safepointMechanism.inline.hpp
> --- a/src/hotspot/share/runtime/safepointMechanism.inline.hpp Mon Jan 
> 28 13:10:15 2019 +0100
> +++ b/src/hotspot/share/runtime/safepointMechanism.inline.hpp Mon Jan 
> 28 14:10:59 2019 +0100
> @@ -82,1 +82,1 @@
> -void SafepointMechanism::block_at_safepoint(JavaThread* thread) {
> +void SafepointMechanism::callback_if_safepoint(JavaThread* thread) {
> @@ -84,1 +84,1 @@
> -    SafepointSynchronize::block(thread);
> +    SafepointSynchronize::block(thread, false);
> diff -r e65cc445234c src/hotspot/share/runtime/thread.cpp
> --- a/src/hotspot/share/runtime/thread.cpp    Mon Jan 28 13:10:15 2019 
> +0100
> +++ b/src/hotspot/share/runtime/thread.cpp    Mon Jan 28 14:10:59 2019 
> +0100
> @@ -298,2 +297,0 @@
> -  block_in_safepoint_check = true;
> -
> diff -r e65cc445234c src/hotspot/share/runtime/thread.hpp
> --- a/src/hotspot/share/runtime/thread.hpp    Mon Jan 28 13:10:15 2019 
> +0100
> +++ b/src/hotspot/share/runtime/thread.hpp    Mon Jan 28 14:10:59 2019 
> +0100
> @@ -788,2 +787,0 @@
> -  bool block_in_safepoint_check;              // to decide whether to 
> block in SS::block or not
> -
>
>
> On 1/28/19 9:42 AM, Patricio Chilano wrote:
>> Hi all,
>>
>> Please review the following patch:
>>
>> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8210832
>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8210832/v01/webrev/
>>
>> The current implementation of native monitors uses a technique that 
>> we name "sneaky locking" to prevent possible deadlocks of the JVM 
>> during safepoints. The implementation of this technique though 
>> introduces a race when a monitor is shared between the VMThread and 
>> non-JavaThreads. This patch aims to solve that problem and at the 
>> same time simplify the code.
>>
>> The proposal is based on the introduction of the new class 
>> PlatformMonitor, which serves as a wrapper for the actual 
>> synchronization primitives in each platform (mutexes and condition 
>> variables). Most of the API calls can thus be implemented as simple 
>> wrappers around PlatformMonitor, adding more assertions and very 
>> little extra metadata.
>> To be able to remove the lock sneaking code and at the same time 
>> avoid deadlocking scenarios, we combine two techniques:
>>
>> -When a JavaThread that has just acquired the lock, detects there is 
>> a safepoint request in the ThreadLockBlockInVM destructor, it 
>> releases the lock before blocking at the safepoint. After resuming 
>> from it, the JavaThread will have to acquire the lock again.
>>
>> - In the ThreadLockBlockInVM constructor for the Monitor::wait() 
>> method, in order to avoid blocking we allow for a possible safepoint 
>> request to make progress but without letting the JavaThread block for 
>> it (since we would be stopped by the destructor anyways). We also do 
>> that for the Monitor::lock() case although no deadlock is being 
>> prevented there.
>>
>> The ThreadLockBlockInVM jacket is a new ThreadStateTransition class 
>> used instead of the ThreadBlockInVM one. This allowed more 
>> flexibility to handle the two techniques mentioned above. Also, 
>> ThreadBlockInVM calls SafepointMechanism::block_if_requested() which 
>> creates some problems when trying to allow safepoints to continue 
>> without stopping, since that method not only checks for safepoints 
>> but also processes handshakes.
>>
>> In terms of performance, benchmarks show very similar results to what 
>> we have now.
>>
>> So far mach5 hs-tier1-6 on Linux, OS X, Windows and Solaris have been 
>> tested.
>>
>> Thanks,
>> Patricio
>>