RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Wed Jan 30 00:24:02 UTC 2019
Hi Per,
On 1/29/19 4:22 AM, Per Liden wrote:
> Hi Patricio,
>
> On 01/28/2019 08:18 PM, Patricio Chilano wrote:
>> 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
>
> I really like that we're ditching our old locking code in favor of
> using pthread_mutex, et al. Nice work!
Thanks! : )
> 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:
>
> 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.
I agree this is a good idea, but since it would make sense to also
rework them at the high-level Monitor/Mutex as David pointed out (this
idea is actually also proposed in the comments of mutex.hpp) what do you
think if I file this as a separate bugid to be worked after we pushed
this patch ?
> 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).
I tried to moved them but there is a small issue in that PlatformMonitor
code needs static methods defined in their current os_*.cpp files
(methods that parse timing structs). I can declare them as public
(cannot move them since they are also used by PlatformEvent and Parker),
but for the Posix version of PlatformMonitor I would also need to do
that with _condAttr and _mutexAttr which are also defined static in that
file and are needed by PlatformMonitor::PlatformMonitor. So not sure
what the right approach is here.
In any case shouldn't we aim to have all synchronization-like classes in
the same file for each platform (something like syncro_posix,
syncro_windows, etc) instead of a separate file for each of them
(semaphore_*, monitor_*, waitbarrier_*, etc). Otherwise seems
PlatformParker and PlatformEvent should also be in their own file.
> 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 ;)
Done!
> src/hotspot/os/posix/os_posix.cpp
> ---------------------------------
> * Destructor missing, to call pthread_(mutex|cond)_destroy().
Done!
> 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.
Done!
> 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).
Done! (There is a destroy function for mutexes but not for condition
variables which apparently do not need to free anything explicitly).
> 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.
Done! I realized there is no need for passing a parameter to
do_preempted() since we already have the in_flight_monitor_adr so I also
made small changes there.
Here is v03 including also Dan and Robbin comments about mutex.cpp and
safepointMechanism.hpp:
Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v03/webrev/
Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v03/inc/webrev/
Running mach tiers1-3. Waiting though on you thoughts about file
organization and deferring Mutex/Monitor rework.
Thanks for looking into this Per!
Thanks,
Patricio
> 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