RFR: 8210832: Remove sneaky locking in class Monitor

Daniel D. Daugherty daniel.daugherty at oracle.com
Sat Feb 2 00:58:16 UTC 2019 

On 2/1/19 7:49 PM, David Holmes wrote:
> <trimming>
>
> On 2/02/2019 7:48 am, Patricio Chilano wrote:
>> On 2/1/19 4:28 PM, coleen.phillimore at oracle.com wrote:
>>>> In regards to the name ThreadLockBlockInVM ... I don't have a good 
>>>> suggestion. 
>>>> ThreadBlockInVMWithSafepointCheckingButOnlyBlockOnTheWayOut is 
>>>> rather unwieldy. ;-) But the "Lock" part really doesn't mean 
>>>> anything. So your suggestion of 
>>>> ThreadBlockinVMWithDeadlockPrevention seems a big improvement to 
>>>> me. :) 
>>>
>>> How about ThreadBlockInVMForLock ?   This answers the question "why" 
>>> this class, vs. "what" this class does.  Since the latter can change. 
>> I like that name. Is that name okay with you David?
>
> Sorry no. "Lock" still adds zero information about when/why you would 
> use this. It's used within Monitor::lock and Monitor::wait so is not 
> specific to "locking" even in that class. This is about blocking in 
> the VM and we need to know what this version does differently to the 
> existing TBIVM. There are lots of different types of "locking" in the 
> VM and we don't use this with them.
>
> Dan can be a tie-breaker on this bikeshed. Grab your brush Dan! ;-)

ThreadBlockInVMBecauseSneakyLockingIsEvil?

ThreadBlockinVMWithDeadlockPrevention is okay.

ThreadBlockinVMWithDeadlockCheck
ThreadBlockinVMWithDeadlockChk
ThreadBlockinVMWithDeadlockCk (David won't like this one :-) )

Just pick a name and run with it... :-)

Dan


>
> David
> -----
>
>
>> Thanks,
>> Patricio
>>> thanks,
>>> Coleen
>>>
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>> On 1/02/2019 3:05 pm, Patricio Chilano wrote:
>>>>> Hi David,
>>>>>
>>>>> On 1/31/19 12:54 AM, David Holmes wrote:
>>>>>> On 31/01/2019 7:37 am, Patricio Chilano wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> On 1/30/19 2:29 AM, David Holmes wrote:
>>>>>>>> Hi Patricio,
>>>>>>>>
>>>>>>>> <trimming>
>>>>>>>>
>>>>>>>> First, thanks for all the many weeks of work you've put into 
>>>>>>>> this, pulling together a number of ideas from different people 
>>>>>>>> to make it all work!
>>>>>>> Thanks! Credit to you for the PlatformMonitor implementation  : )
>>>>>>
>>>>>> :) Nothing innovative there.
>>>>>>
>>>>>>>> I've only got a few minor comments/suggestions.
>>>>>>>>
>>>>>>>> On 30/01/2019 10:24 am, Patricio Chilano wrote:
>>>>>>>>> Full: 
>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/webrev/
>>>>>>>>> Inc: 
>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/inc/webrev/
>>>>>>>>>
>>>>>>>>
>>>>>>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>>>>>>>
>>>>>>>> I'm very unclear how ThreadLockBlockInVM differs from 
>>>>>>>> ThreadBlockInVM. You've duplicated a lot of complex code which 
>>>>>>>> is masking the actual difference between the two wrappers to 
>>>>>>>> me. It seems to me that an extra arg to transition_and_fence 
>>>>>>>> should allow you to handle the new behaviour without having to 
>>>>>>>> duplicate so much of this code. In any case the semantics of 
>>>>>>>> ThreadLockBlockInVM needs to be described.
>>>>>>> I could do it with one extra argument, but I would need to add 
>>>>>>> two extra branches in transition_and_fence(), one to decide if 
>>>>>>> I'm in the Monitor case to avoid calling 
>>>>>>> SafepointMechanism::block_if_requested() directly and another 
>>>>>>> one to actually decide if I'm transitioning in or out, since the 
>>>>>>> actions to perform are different. I think it is easier to read 
>>>>>>> without adding new conditionals, and also we will save those 
>>>>>>> extra branches, but if you think it's better this way I can 
>>>>>>> change it.
>>>>>>
>>>>>> I would like something that tells me more clearly how this new 
>>>>>> transition helper differs from the existing TBIVM. Sharing the 
>>>>>> code between them and using different args would be one way. 
>>>>>> Documenting the difference in comments would be another. Your 
>>>>>> choice.
>>>>> Ok, I added a description on top of TLBIVM.
>>>>>
>>>>>>>> Also I'm unclear what the "Lock" in ThreadLockBlockInVM 
>>>>>>>> actually refers to. I find the name quite jarring to read.
>>>>>>> What about changing it to ThreadBlockinMonitor?
>>>>>>
>>>>>> That's not quite conveying the semantics. The problem is that the 
>>>>>> semantics we are changing compared to TBIVM are not evident in 
>>>>>> the TBIVM name. If TBIVM was actually 
>>>>>> ThreadBlockInVMWithSafepointBlocking, then this new transition 
>>>>>> would obviously be ThreadBlockInVMWithoutSafepointBlocking - but 
>>>>>> perhaps that lengthy, but clear name would be okay anyway?
>>>>> Not convinced on that name since we are blocking at safepoints in 
>>>>> the destructor. Based on the comments I added how about 
>>>>> ThreadBlockinVMWithDeadlockPrevention ? or 
>>>>> ThreadBlockinVMWithPreemption? (as in eliminate one of the 
>>>>> conditions for deadlocks).
>>>>>
>>>>>>>> On the subject of naming, do_preempt and preempt_by_safepoint 
>>>>>>>> don't really convey to me what happens - what is being 
>>>>>>>> "preempted" here? I would suggest a more direct 
>>>>>>>> Monitor::release_for_safepoint
>>>>>>> Changed.
>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Logging: why "nativemonitor"? The logging in mutex.cpp doesn't 
>>>>>>>> relate to a "native" monitor?? Actually I'm not even sure if we 
>>>>>>>> need bother at all with the one logging statement that is present.
>>>>>>> I added it to eventually track unbounded try locks. Not sure I 
>>>>>>> follow you with the name, isn't that how we name this monitors? 
>>>>>>> I tried to differentiate them from Java monitors. What about 
>>>>>>> just "monitor"?
>>>>>>
>>>>>> How about vmmonitor ?
>>>>> Ok, changed.
>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>>>>
>>>>>>>> void Monitor::lock_without_safepoint_check(Thread * self) {
>>>>>>>>   // Ensure that the Monitor does not require or allow 
>>>>>>>> safepoint checks.
>>>>>>>>
>>>>>>>> The comment there should only say "not require".
>>>>>>> Done.
>>>>>>>
>>>>>>>> void Monitor::preempt_by_safepoint() {
>>>>>>>>   _lock.unlock();
>>>>>>>> }
>>>>>>>>
>>>>>>>> Apart from renaming this as suggested above, aren't there any 
>>>>>>>> suitable assertions we should have here? safepoint-in-progress 
>>>>>>>> or handshake-in-progress? _owner == Thread::current?
>>>>>>> Ok, I added an assertion that owner should be NULL. Asserting 
>>>>>>> safepoint-in-progress does not really work because _state could 
>>>>>>> change to _not_synchronized right after you checked for it in 
>>>>>>> TLBIVM.
>>>>>>
>>>>>> Okay.
>>>>>>
>>>>>>>> Nit:
>>>>>>>>
>>>>>>>> assert(_owner == Thread::current(), "should be equal: owner=" 
>>>>>>>> INTPTR_FORMAT
>>>>>>>>                    ", self=" INTPTR_FORMAT, p2i(_owner), 
>>>>>>>> p2i(Thread::current()));
>>>>>>>>
>>>>>>>> with Dan's enhanced assertions there's an indentation issue. 
>>>>>>>> The second line should indent to the first comma, but that will 
>>>>>>>> make the second line extend way past 80 columns.
>>>>>>>>
>>>>>>>> Also you could factor that assertion for 
>>>>>>>> _owner==Thread::current() into its own function or macro to 
>>>>>>>> avoid the repetition.
>>>>>>> Corrected indentation based on Dan's reply to align with _owner.
>>>>>>
>>>>>> I though it should indent to the comma because it is a 
>>>>>> continuation of the same argument being passed to the assert 
>>>>>> "function". But I'm okay with Dan's suggestion.
>>>>>>
>>>>>> Factoring it into its own little function or macro would still be 
>>>>>> good to avoid the repetition.
>>>>> Ok, added new function assert_owned_by_self(). I could change it 
>>>>> to assert_owner(Thread*) and use it for the NULL case too unifying 
>>>>> the printed messages to something like "invalid owner: owner=" 
>>>>> INTPTR_FORMAT ", should be:" INTPTR_FORMAT. What do you think?
>>>>>
>>>>>>>>  OSThreadWaitState osts(self->osthread(), false /* not 
>>>>>>>> Object.wait() */);
>>>>>>>>
>>>>>>>> This needs to be returned to its original place as per Dan's 
>>>>>>>> comments.
>>>>>>> Done.
>>>>>>>
>>>>>>>>     } else {
>>>>>>>>       Monitor::lock(self);
>>>>>>>>     }
>>>>>>>>
>>>>>>>> You don't need Monitor:: here
>>>>>>> Removed.
>>>>>>>
>>>>>>>> // Temporary JVM_RawMonitor* support. A raw monitor can just be 
>>>>>>>> a PlatformMonitor now.
>>>>>>>>
>>>>>>>> This needs to be resolved before committing. Some of the 
>>>>>>>> existing commentary on what raw monitors are needs to be 
>>>>>>>> retained. Not clear if we need to set the _owner field or can 
>>>>>>>> just skip it.
>>>>>>> Is it okay if I keep the following comments?
>>>>>>>
>>>>>>> // Yet another degenerate version of Monitor::lock() or 
>>>>>>> lock_without_safepoint_check()
>>>>>>> // jvm_raw_lock() and _unlock() can be called by non-Java 
>>>>>>> threads via JVM_RawMonitorEnter.
>>>>>>> //
>>>>>>> // There's no expectation that JVM_RawMonitors will interoperate 
>>>>>>> properly with the native
>>>>>>> // Mutex-Monitor constructs.  We happen to implement 
>>>>>>> JVM_RawMonitors in terms of
>>>>>>> // native Mutex-Monitors simply as a matter of convenience.
>>>>>>
>>>>>> Yep that's perfect. And as a future RFE we can replace them with 
>>>>>> direct use of PlatformMonitor (or PlatformMutex).
>>>>>>
>>>>>>>
>>>>>>> I could keep setting the owner as _owner = 
>>>>>>> Thread::current_or_null() in jvm_raw_lock(), at least it 
>>>>>>> wouldn't hurt.
>>>>>>
>>>>>> It's useful for checking usage errors, but we won't have that if 
>>>>>> we replace with PlatformMonitor, so may as well drop it now IMO.
>>>>> Ok, I added asserts that _owner should be NULL after acquiring it 
>>>>> and before releasing it though.
>>>>>
>>>>>>>> Monitor::~Monitor() {
>>>>>>>>   assert(_owner == NULL, "should be NULL: owner=" 
>>>>>>>> INTPTR_FORMAT, p2i(_owner));
>>>>>>>> }
>>>>>>>>
>>>>>>>> Will this automatically result in the PlatformMonitor 
>>>>>>>> destructor being called?
>>>>>>> Yes, should I add a comment to make it clear that 
>>>>>>> ~PlatformMonitor() will be executed?
>>>>>>
>>>>>> No need - assume other people have a better understanding of C++ 
>>>>>> than I do :)
>>>>>
>>>>> Below is version v04, which also contains a correction pointed out 
>>>>> off-list by Robbin to do a local poll first in 
>>>>> SafepointMechanism::callback_if_safepoint() if we are using local 
>>>>> polls. Since the thread local poll is armed after changing _state 
>>>>> to _synchronizing, if we only do a global poll in the TLBIVM 
>>>>> constructor we could detect there is a safepoint in progress and 
>>>>> callback but when coming back in the destructor 
>>>>> SafepointMechanism::should_block() could miss detecting the 
>>>>> safepoint in progress since that method checks first for local polls.
>>>>>
>>>>>
>>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/
>>>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/inc/
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Patricio
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks for looking into this! Waiting on your comments to send v04.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Patricio
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>
>>>>>
>>>
>>

More information about the hotspot-dev mailing list