RFR: 8210832: Remove sneaky locking in class Monitor

David Holmes david.holmes at oracle.com
Sat Feb 2 00:49:38 UTC 2019 

<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! ;-)

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