RFR: 8210832: Remove sneaky locking in class Monitor

[David Holmes]

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.

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

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

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

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

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

Thanks,
David

> 
> 
> Thanks for looking into this! Waiting on your comments to send v04.
> 
> Thanks,
> Patricio
>> ---
>>
>> Thanks,
>> David
>> -----
>>
>