RFR: 8210832: Remove sneaky locking in class Monitor

Patricio Chilano patricio.chilano.mateo at oracle.com
Fri Feb 1 06:31:46 UTC 2019 

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'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