RFR: 8210832: Remove sneaky locking in class Monitor

David Holmes david.holmes at oracle.com
Sat Feb 2 00:31:07 UTC 2019 

On 2/02/2019 7:13 am, coleen.phillimore at oracle.com wrote:
> 
> 
> On 2/1/19 1:31 AM, Patricio Chilano wrote:
>> Hi David,
>>
>> On 2/1/19 12:34 AM, David Holmes wrote:
>>> Hi Patricio,
>>>
>>> Comments on inc v4:
>>>
>>> src/hotspot/share/runtime/safepointMechanism.inline.hpp
>>>
>>> +   if(!uses_thread_local_poll() || local_poll_armed(thread)) {
>>>
>>> Need space after if.
>>>
>>> I trust Robbin's judgement that this is the right way to express 
>>> this, but if ordering is so critical it should be documented.
>> He didn't propose that exact line but local_poll_armed() is a load 
>> acquire so should be okay. But let's see what Robbin says.
>>
>>> ---
>>>
>>> src/hotspot/share/runtime/mutex.hpp/.cpp
>>>
>>> assert_owned_by_self doesn't need to be a member function, it can 
>>> just be a file static in the .cpp file. No need to clutter the API.
>>>
>>> Using a version that takes the owner thread is a good idea for 
>>> checking the NULL case too.
>> Done.
>>
>>> ---
>>>
>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>>
>>> I like the comment block - thanks! A couple of minor things:
>>>
>>> + // are trying to acquire. This will be used to access and release 
>>> the monitor in case needed to avoid
>>>
>>> s/in case/if/
>> Done.
>>
>>> + // the monitor that is in the process of being acquired (only 
>>> setting the owner is missing).
>>>
>>> I think the part in parentheses can be dropped - it reads like you 
>>> should be setting the owner but have chosen not to for some reason.
>> Done.
>>
>>> + // The logic of this class could have been integrated into a more 
>>> general ThreadBlockInVM. By using a
>>> + // separate class though, we avoid executing branches that would 
>>> otherwise be needed to identify each
>>> + // case.
>>>
>>> Not necessary IMHO but up to you whether to keep or drop.
>> Dropped.
>>
>>> 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. :)
>> ThreadBlockInVMWithSafepointCheckingButOnlyBlockOnTheWayOut was the 
>> right one, too bad it's so long =D. Ok, I''ll leave 
>> ThreadBlockInVMWithDeadlockPrevention, if anybody comes up with a 
>> better one I can change it.
> 
> I want ThreadLockBlockInVM - yah it looks the same as ThreadBlockInVM 
> but I'd rather the name say that it's used for Locks vs some specifics 
> about what it does.  I hope all the jackets prevent deadlock!  So my 
> vote is to leave the name as in version 1.

but its NOT used for "Locks" it's used for Monitor/Mutex.  "locks" is 
far too generic given all the different type of locking that exists.

David

> Coleen
> 
>>
>> I'll wait for Robbin then to send v05.
>>
>> Thanks!
>> Patricio
>>
>>> 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