RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Wed Jan 30 14:49:56 UTC 2019
Hi Per,
On 1/30/19 6:26 AM, Per Liden wrote:
> Hi Patricio,
>
> On 01/30/2019 01:24 AM, Patricio Chilano wrote:
>> 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 ?
>
> Sure, that can be done in a separate follow up 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.
>
> Keeping things in separate files can make sense if these things can be
> used standalone. A plain mutex (just like the plain semaphore we have)
> can come handy in many places where you just want that mutex, without
> having to drag in other classes or the whole os layer. Keeps
> dependencies under control, reduces compile times, etc.
Ok. If you don't mind then I can do that in the follow up RFE for the
Monitor/Mutex rework after JDK-8217843 since David mentioned is working
on moving things too.
>>> 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/
>
> What kind of performance measurements have been done on this patch?
>
> I took your v03 patch for a spin in SPECjbb2015 (with ZGC enabled) and
> did not notice any obvious regressions in either throughput nor latency.
I run SPECjbb2005, SPECjvm2008, SPECjvm98, SPECjbb2015 and a couple of
other small benchmarks in Linux_x64 and Windows_x64 with the version I
had before the reviews and as you said performance overall seems to stay
the same.
Thanks,
Patricio
> cheers,
> Per
>
>>
>> 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