RFR: 8210832: Remove sneaky locking in class Monitor
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Jan 28 21:29:24 UTC 2019
On 1/28/19 2: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
src/hotspot/os/posix/os_posix.cpp
No comments.
src/hotspot/os/posix/os_posix.hpp
No comments.
src/hotspot/os/solaris/os_solaris.cpp
No comments.
src/hotspot/os/solaris/os_solaris.hpp
No comments.
src/hotspot/os/windows/os_windows.cpp
L5309: else {
L5310: DWORD err = GetLastError();
L5311: assert(err == ERROR_TIMEOUT,
"SleepConditionVariableCS: %ld:", err);
L5312: }
nit - please reduce indent by 2 spaces.
src/hotspot/os/windows/os_windows.hpp
No comments.
src/hotspot/share/logging/logTag.hpp
No comments.
src/hotspot/share/runtime/interfaceSupport.inline.hpp
No comments.
src/hotspot/share/runtime/mutex.cpp
L42: // Clear unhandled oops in JavaThreads so we get a crash
right away
nit - please add '.' and the end of the sentence.
L128: assert(_owner == Thread::current(), "invariant");
L134: assert(_owner == Thread::current(), "invariant");
L139: assert(_owner == Thread::current(), "invariant");
Please consider the following for better diagnostics:
assert(_owner == Thread::current(), "should be equal:
owner=" INTPTR_FORMAT
", current=" INTPTR_FORMAT, p2i(_owner),
p2i(Thread::current()));
Sorry I missed this one in the preliminary review.
L155: assert(_owner == self, "invariant");
assert(_owner == self, "should be equal: owner=" INTPTR_FORMAT
", self=" INTPTR_FORMAT, p2i(_owner), p2i(self));
L205: jt->java_suspend_self();
L206: }
L207: }
L208:
L209: if (in_flight_monitor != NULL) {
L210: // Conceptually reestablish ownership of the lock.
L211: assert(_owner == NULL, "should be NULL: owner="
INTPTR_FORMAT, p2i(_owner));
L212: set_owner(self);
L213: } else {
L214: lock();
L215: }
The lock reacquire on L214 used to be done after the
java_suspend_self() on L205 which is inside the block
context for the ThreadLockBlockInVM and OSThreadWaitState
helps. If the lock() blocks due to a racing thread, then
the calling JavaThread won't have the right thread state
of OS thread wait state, etc...
Also after lock() on L214, you never call set_owner(self)
so the ownership is not complete for that relocated code.
You won't need the set_owner(self) call if you move the
lock() on L214 back to after java_suspend_self().
I'm not sure how I missed these two things in the preliminary
review. I can't go back to that webrev since it looks like the
preliminary webrev has been overwritten.
So after java_suspend_self(), you have to re-lock() so that
a potential block on that re-lock has the right states. This
also means that this line has to be deleted:
L204 in_flight_monitor = NULL;
so that ThreadLockBlockInVM destructor can do the right thing
if the thread is suspended and then relocks.
src/hotspot/share/runtime/mutex.hpp
No comments.
src/hotspot/share/runtime/mutexLocker.hpp
No comments.
src/hotspot/share/runtime/safepoint.cpp
No comments.
src/hotspot/share/runtime/safepoint.hpp
No comments.
src/hotspot/share/runtime/safepointMechanism.hpp
L75, L78, and L81 - nit - need a period at the end of the sentence.
src/hotspot/share/runtime/safepointMechanism.inline.hpp
No comments.
src/hotspot/share/runtime/thread.cpp
No comments.
src/hotspot/share/runtime/thread.hpp
No comments.
Dan
> 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