RFR: 8210832: Remove sneaky locking in class Monitor
Per Liden
per.liden at oracle.com
Wed Jan 30 11:26:30 UTC 2019
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.
>
>> 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.
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