RFR: 8210832: Remove sneaky locking in class Monitor
Robbin Ehn
robbin.ehn at oracle.com
Tue Jan 29 10:00:12 UTC 2019
Hi Per/Patricio,
On 1/29/19 10:22 AM, Per Liden wrote:
> General comment
> ----------------
> I think Mutex to be a plain mutex and not come with the baggage of having a
> conditional variable. With this new code, it seems we're in a really good
> position to make that happen. I.e. something like this:
This is a good idea. I suppose it enables us to remove zlock ?
Also comment for callback_if_safepoint should be updated.
81 // Blocks a thread until safepoint is completed
82 static inline void callback_if_safepoint(JavaThread* thread);
Thanks, Robbin
>
> class PlatformMutex {
> protected:
> pthread_mutex_t _mutex;
>
> public:
> PlatformMutex();
> ~PlatformMutex();
>
> void lock();
> void unlock();
> bool try_lock();
> };
>
> class PlatformMonitor : public PlatformMutex {
> private:
> pthread_cond_t _cond;
>
> public:
> PlatformMonitor();
> ~PlatformMonitor();
>
> int wait(jlong millis);
> void notify();
> void notify_all();
> };
>
> It might be that we want to do that as a separate step later instead of
> including it in this patch. But I think we should try to get there.
>
>
> src/hotspot/os/posix/os_*.[ch]pp
> ---------------------------------
> * I'd suggest that we place the PlatformMonitor class in a separate file (like
> src/hotspot/os/posix/monitor_posix.cpp), just like we have done with Semaphore
> (in src/hotspot/os/posix/semaphore_posix.cpp).
>
>
> src/hotspot/os/posix/os_posix.hpp
> src/hotspot/os/solaris/os_solaris.hpp
> src/hotspot/os/windows/os_windows.hpp
> -------------------------------------
> * Please make _mutex/_cond plain variables, instead of arrays of 1. That's just
> ugly ;)
>
>
> src/hotspot/os/posix/os_posix.cpp
> ---------------------------------
> * Destructor missing, to call pthread_(mutex|cond)_destroy().
>
>
> src/hotspot/os/solaris/os_solaris.hpp
> -------------------------------------
> * Not sure if there's a good reason to have the constructor be inlined here. I'd
> suggest moving it to the cpp file.
>
> * Destructor missing.
>
>
> src/hotspot/os/windows/os_windows.cpp
> -------------------------------------
> * Destructor missing (I'm not too familiar with the windows API but I assume
> there's a destroy function we should call here).
>
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp
> -----------------------------------------------------
> * Move "private:" above monitor_adr;
>
> 289 class ThreadLockBlockInVM : public ThreadStateTransition {
> 290 Monitor** monitor_adr;
> 291 private:
> 292 void do_preempted(Monitor** in_flight_monitor_adr) {
>
> * monitor_adr should be _monitor_adr, or maybe even _in_flight_monitor_adr to
> better match the name of the argument.
>
>
> cheers,
> Per
>
>> 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
>>>>
>>
More information about the hotspot-dev
mailing list