RFR: 8210832: Remove sneaky locking in class Monitor

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

Hi Coleen,

On 2/1/19 4:28 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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.
>>
>> ---
>>
>> 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.
>>
>> ---
>>
>> 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/
>>
>> + //   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.
>>
>> + // 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.
>>
>> 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?

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