RFR: 8210832: Remove sneaky locking in class Monitor

Fri Feb 1 05:05:26 UTC 2019

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